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

refactor: move private RTL direction getter to DirMixin #5006

Merged
merged 3 commits into from
Nov 14, 2022
Merged

refactor: move private RTL direction getter to DirMixin #5006

merged 3 commits into from
Nov 14, 2022

Conversation

web-padawan
Copy link
Member

Description

Moved __isRTL getter to DirMixin to reduce code duplication in components that use this mixin.
Also removed attributeChangedCallback() from vaadin-grid, it's not needed since #2396.

Type of change

  • Refactor

@web-padawan web-padawan requested review from tomivirkki and vursen and removed request for tomivirkki and vursen November 14, 2022 14:29
@web-padawan web-padawan marked this pull request as draft November 14, 2022 14:41
@web-padawan
Copy link
Member Author

Seems like vaadin-grid logic and tests need to be adjusted, converting to draft for now, will work on it later.

@NikolayySt NikolayySt mentioned this pull request Nov 14, 2022
Closed
9 tasks
@web-padawan web-padawan marked this pull request as ready for review November 14, 2022 15:08
@web-padawan
Copy link
Member Author

UPD: decided to not change grid tests for now, they could be adjusted later (currently, some of them are setting dir on the <vaadin-grid> element itself, not on the <html> element, but using the latter makes things more complicated).

sissbruecker
sissbruecker approved these changes Nov 14, 2022
View reviewed changes
Copy link
Contributor

@sissbruecker sissbruecker left a comment

Choose a reason for hiding this comment

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

Looks good, note that there are also several occurrences of getAttribute('dir') that could possibly be replaced with the new getter:

Bildschirmfoto 2022-11-14 um 18 11 52

@web-padawan
Copy link
Member Author

@sissbruecker Thanks, updated most of those occurrences in other components.

@sonarcloud
Copy link

sonarcloud bot commented Nov 14, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@web-padawan web-padawan merged commit cdc678e into master Nov 14, 2022
@web-padawan web-padawan deleted the refactor/is-rtl-getter branch November 14, 2022 18:04
@vaadin-bot
Copy link
Collaborator

Hi @web-padawan , this commit cannot be picked to 23.3 by this bot, can you take a look and pick it manually?
Error Message: Error: Command failed: git cherry-pick cdc678e
error: could not apply cdc678e... refactor: move private RTL direction getter to DirMixin (#5006)
hint: After resolving the conflicts, mark them with
hint: "git add/rm ", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

@vaadin-bot
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sissbruecker sissbruecker sissbruecker approved these changes

@vursen vursen vursen approved these changes

Assignees
No one assigned
Labels
need to pick manually 23.3 prerelease version for Vaadin 24.0.0 Released with Vaadin 24.0.0.alpha4 target/23.3
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@web-padawan @vaadin-bot @sissbruecker @vursen