Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: implement disable on click for menu item #6815

Merged
merged 12 commits into from
Nov 18, 2024
Merged

feat: implement disable on click for menu item #6815

merged 12 commits into from
Nov 18, 2024

Conversation

ugur-vaadin
Copy link
Contributor

@ugur-vaadin ugur-vaadin commented Nov 12, 2024 •
edited
Loading

Description

This PR

  • implements disable on click behaviour for MenuItem with the following API:
    • void setDisableOnClick(boolean disableOnClick)
    • boolean isDisableOnClick()
  • extracts disable on click logic to a controller DisableOnClickController

Fixes #1472

Type of change

  • Bugfix
  • Feature

Checklist

  • I have read the contribution guide: https://vaadin.com/docs/latest/contributing/overview
  • I have added a description following the guideline.
  • The issue is created in the corresponding repository and I have referenced it.
  • I have added tests to ensure my change is effective and works as intended.
  • New and existing tests are passing locally with my change.
  • I have performed self-review and corrected misspellings.
  • Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

@ugur-vaadin ugur-vaadin force-pushed the feat-implement-disable-on-click-for-menu-item branch 2 times, most recently from 52ec96e to 1712228 Compare November 13, 2024 19:06
@ugur-vaadin ugur-vaadin marked this pull request as ready for review November 13, 2024 19:06
sissbruecker
sissbruecker reviewed Nov 14, 2024
View reviewed changes
...components-base/src/main/java/com/vaadin/flow/component/shared/DisableOnClickController.java Outdated Show resolved Hide resolved
...rent/vaadin-button-flow/src/test/java/com/vaadin/flow/component/button/tests/ButtonTest.java Outdated Show resolved Hide resolved
...n-tests/src/test/java/com/vaadin/flow/component/contextmenu/it/MenuItemDisableOnClickIT.java Outdated Show resolved Hide resolved
...tests/src/main/java/com/vaadin/flow/component/contextmenu/it/MenuItemDisableOnClickView.java Outdated Show resolved Hide resolved
...components-base/src/main/java/com/vaadin/flow/component/shared/DisableOnClickController.java Outdated Show resolved Hide resolved
...tests/src/main/java/com/vaadin/flow/component/contextmenu/it/MenuItemDisableOnClickView.java Outdated Show resolved Hide resolved
@ugur-vaadin ugur-vaadin force-pushed the feat-implement-disable-on-click-for-menu-item branch from fc7cdbb to 2b8bbad Compare November 15, 2024 12:28
@ugur-vaadin
Copy link
Contributor Author

Added a few more tests to the controller, and cleaned up the listener initialization logic.

sissbruecker
sissbruecker reviewed Nov 18, 2024
View reviewed changes
vaadin-flow-components-shared-parent/vaadin-flow-components-base/pom.xml
Comment on lines +31 to +35
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
<scope>test</scope>
</dependency>
Copy link
Contributor

@sissbruecker sissbruecker Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't look like this is needed anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is still used in 3 of the unit tests (the ones for initializing disabled on click).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, sorry, I was looking at the wrong module.

...s-base/src/main/java/com/vaadin/flow/component/shared/internal/DisableOnClickController.java Outdated
* if server-side handling takes some time.
*/
private void initDisableOnClick() {
if (initializedInCurrentRoundTrip) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name is potentially confusing, as this doesn't reset if the component is already attached. It also looks like it's now possible to add multiple attach listeners.

Maybe always early return this method if there is already an attach listener, because then it never needs to run again.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the names and refactored it to return early.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed the changes.

ugur-vaadin and others added 12 commits November 18, 2024 14:24
@ugur-vaadin
feat: implement disable on click for menu item
538ebba
@ugur-vaadin
fix: defer setting disabled
27fbbd0
@ugur-vaadin
test: add unit tests for controller
c8b0409
@ugur-vaadin
test: fix test class
b2d2143
@ugur-vaadin
fix: check if element is there on disableonclick init and cleanup
5891852
@ugur-vaadin
test: fix button tests
1e68439
@ugur-vaadin @vursen
Update vaadin-flow-components-shared-parent/vaadin-flow-components-ba…
a8b6cc9 
…se/src/main/java/com/vaadin/flow/component/shared/internal/DisableOnClickController.java

Co-authored-by: Sergey Vinogradov <mr.vursen@gmail.com>
@ugur-vaadin @vursen
Update vaadin-flow-components-shared-parent/vaadin-flow-components-ba…
02cbe94 
…se/src/main/java/com/vaadin/flow/component/shared/internal/DisableOnClickController.java

Co-authored-by: Sergey Vinogradov <mr.vursen@gmail.com>
@ugur-vaadin @vursen
Update vaadin-flow-components-shared-parent/vaadin-flow-components-ba…
e229e82 
…se/src/main/java/com/vaadin/flow/component/shared/internal/DisableOnClickController.java

Co-authored-by: Sergey Vinogradov <mr.vursen@gmail.com>
@ugur-vaadin
refactor: update disable on click init logic and add tests
a509475
@ugur-vaadin
refactor: rename and return early when initializing
130c3ad
@ugur-vaadin
test: reuse elements and simplify disabled asseertion
eacfffd
@ugur-vaadin ugur-vaadin force-pushed the feat-implement-disable-on-click-for-menu-item branch from 52861cc to eacfffd Compare November 18, 2024 12:24
@ugur-vaadin ugur-vaadin enabled auto-merge (squash) November 18, 2024 12:30
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@ugur-vaadin ugur-vaadin merged commit 8b00ed7 into main Nov 18, 2024
4 of 5 checks passed
@ugur-vaadin ugur-vaadin deleted the feat-implement-disable-on-click-for-menu-item branch November 18, 2024 13:11
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.6.0.alpha4 and is also targeting the upcoming stable 24.6.0 version.

@web-padawan web-padawan mentioned this pull request Nov 20, 2024
Merged
@sissbruecker sissbruecker mentioned this pull request Jan 8, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DiegoCardoso DiegoCardoso DiegoCardoso left review comments

@vursen vursen vursen approved these changes

@sissbruecker sissbruecker sissbruecker approved these changes

Assignees
No one assigned
Labels
prerelease version for Vaadin 24.6.0 Released with Vaadin 24.6.0.alpha4
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

MenuItem DisableOnClick
5 participants
@ugur-vaadin @vaadin-bot @DiegoCardoso @sissbruecker @vursen