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

Correctly align overlay in RTL mode #584

Merged
merged 2 commits into from
Nov 20, 2018
Merged

Correctly align overlay in RTL mode #584

merged 2 commits into from
Nov 20, 2018

Conversation

samiheikki
Copy link
Contributor

@samiheikki samiheikki commented Jul 4, 2018

Fixes #421


This change is Reviewable

Copy link
Contributor

@platosha platosha left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @samiheikki and @YuriyVaadin)


src/vaadin-date-picker-mixin.html, line 504 at r1 (raw file):

        if (rightAlign) {
          if (getComputedStyle(this._inputElement).getPropertyValue('direction') === 'rtl') {
            this.$.overlay.removeAttribute('right-aligned');

Does not make sense to me to remove right-aligned attribute. This case is still right-aligned in fact, so we should keep right-aligned attr in place. We should rather fix right-aligned to do the right thing in CSS (pun intended).

The problem is that in CSS we have :host([right-aligned]) { align-items: flex-end; } in the overlay styles, with flex-end meaning the left side in RTL mode. In this particular use case we rather need flex-right there (not a thing, but I wish we could), because we are bound to use right anyway, because the rest of the positioning logic for the overlay uses the right property.

#useThePlatform way of fixing the CSS would be to use append :host([right-aligned]:dir(rtl)) { align-items: flex-start; } in CSS. However, that wouldn’t work because the :dir() CSS selector support is poor. So I suggest:

this.$.overlay.setAttribute('dir',
  getComputedStyle(this._inputElement).getPropertyValue('direction')
);
/* vaadin-date-picker-overlay-styles */
:host([right-aligned]) { align-items: flex-end; }
:host([right-aligned][dir="rtl"]) { align-items: flex-start; }

Alternatively, we could use the Polymer.DirMixin as a workaround for :dir CSS selector as well, but I think that would be a slight overkill, the Vanilla workaround suggested above is enough.

@samiheikki samiheikki force-pushed the fix/rtl branch 2 times, most recently from ec97686 to cf3d6b1 Compare August 25, 2018 12:52
Copy link
Contributor Author

@samiheikki samiheikki left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @platosha and @YuriyVaadin)


src/vaadin-date-picker-mixin.html, line 504 at r1 (raw file):

Previously, platosha (Anton Platonov) wrote…

Does not make sense to me to remove right-aligned attribute. This case is still right-aligned in fact, so we should keep right-aligned attr in place. We should rather fix right-aligned to do the right thing in CSS (pun intended).

The problem is that in CSS we have :host([right-aligned]) { align-items: flex-end; } in the overlay styles, with flex-end meaning the left side in RTL mode. In this particular use case we rather need flex-right there (not a thing, but I wish we could), because we are bound to use right anyway, because the rest of the positioning logic for the overlay uses the right property.

#useThePlatform way of fixing the CSS would be to use append :host([right-aligned]:dir(rtl)) { align-items: flex-start; } in CSS. However, that wouldn’t work because the :dir() CSS selector support is poor. So I suggest:

this.$.overlay.setAttribute('dir',
  getComputedStyle(this._inputElement).getPropertyValue('direction')
);
/* vaadin-date-picker-overlay-styles */
:host([right-aligned]) { align-items: flex-end; }
:host([right-aligned][dir="rtl"]) { align-items: flex-start; }

Alternatively, we could use the Polymer.DirMixin as a workaround for :dir CSS selector as well, but I think that would be a slight overkill, the Vanilla workaround suggested above is enough.

Done. And thanks for the detailed feedback.

Copy link
Contributor

@platosha platosha left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 3 files at r2, 1 of 1 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yuriy-fix)

@yuriy-fix yuriy-fix merged commit 6269fc6 into master Nov 20, 2018
@web-padawan web-padawan deleted the fix/rtl branch November 20, 2018 10:55
@samiheikki
Copy link
Contributor Author

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants