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.

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

refactor: replace global focusin listener with focusout check #7858

Merged
merged 3 commits into from
Sep 26, 2024

Conversation

web-padawan
Copy link
Member

Description

Depends on #7855

This PR simplifies the logic in vaadin-grid-pro that was specifically introduced for handling moving focus from the <input> to the vaadin-date-picker-overlay-content, see the following PRs in the old repo:

Type of change

  • Refactor

Note

I have moved one of the tests to the new test suite for grid-pro with custom editors and removed the old integration test. It was using rather complicated setup with vaadin-dialog since the workaround was specifically checking for the tag name of the overlay where the focus is moving, and this is no longer the case (we now rely on _shouldRemoveFocus() instead).

@web-padawan web-padawan force-pushed the refactor/grid-pro-should-remove-focus branch from 7188843 to db59e2f Compare September 24, 2024 13:34
@web-padawan
Copy link
Member Author

web-padawan commented Sep 24, 2024

UPD: looks like there might be a regression here when wrapping vaadin-date-picker in vaadin-custom-field.
This could be also reproduced by using the setup from the old test where div is used as a wrapper.

I'll investigate if we could somehow detect this case to get the actual editor component for the given cell (currently, for the custom editor type the cell._content.firstElementChild is used and at least on focusout it's wrong).

@web-padawan web-padawan force-pushed the refactor/grid-pro-should-remove-focus branch from 06b3795 to 745e03a Compare September 25, 2024 09:25
@web-padawan web-padawan marked this pull request as ready for review September 25, 2024 09:25
@web-padawan web-padawan force-pushed the refactor/grid-pro-should-remove-focus branch from 745e03a to b74f8a5 Compare September 25, 2024 09:26
@web-padawan web-padawan force-pushed the refactor/grid-pro-should-remove-focus branch from b74f8a5 to 2d5c3ca Compare September 25, 2024 09:26
Copy link

sonarcloud bot commented Sep 25, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
24.6% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

Copy link
Contributor

@vursen vursen left a comment

Choose a reason for hiding this comment

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

LGTM.

This refactoring appears to eliminate the need for the internal-tab event. I tested custom-field as a custom editor in grid-pro, and tabbing between its sub-fields now works correctly even without that event in place. Should we consider removing support for this event from grid-pro and deprecating it in custom-field?

@web-padawan
Copy link
Member Author

Should we consider removing support for this event from grid-pro and deprecating it in custom-field?

Good point. I'll create a separate PR for it and add custom-field to integration tests to verify it's not needed.

@web-padawan web-padawan merged commit fb8144f into main Sep 26, 2024
8 of 9 checks passed
@web-padawan web-padawan deleted the refactor/grid-pro-should-remove-focus branch September 26, 2024 07:06
web-padawan added a commit that referenced this pull request Sep 26, 2024
…#7867)

Co-authored-by: Serhii Kulykov <iamkulykov@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants