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

Fix popovers position in RTL languages #12117

Merged
merged 1 commit into from Nov 20, 2018

Conversation

Projects
None yet
2 participants
@youknowriad
Contributor

youknowriad commented Nov 20, 2018

closes #7660

Popover positioning is RTL agnostic which means the popover styles should be RTL agnostic too :).

Testing instructions

  • Repeat instructions from #7660

@youknowriad youknowriad added this to the 4.5 milestone Nov 20, 2018

@youknowriad youknowriad self-assigned this Nov 20, 2018

@youknowriad youknowriad requested a review from WordPress/gutenberg-core Nov 20, 2018

@@ -4,6 +4,7 @@
*/
const HEIGHT_OFFSET = 10; // used by the arrow and a bit of empty space
const isMobileViewport = () => window.innerWidth < 782;
const isRtl = () => document.documentElement.dir === 'rtl';

This comment has been minimized.

@youknowriad

youknowriad Nov 20, 2018

Contributor

There might be a better way to get this information in the popover (using context or something else). But this will do it for now.

This comment has been minimized.

@aduth

aduth Nov 20, 2018

Member

We have the information from the editor settings wp.data.select( 'core/editor' ).getEditorSettings().isRTL

This comment has been minimized.

@youknowriad

youknowriad Nov 20, 2018

Contributor

yes, but this is the components package which should be generic.

This comment has been minimized.

@aduth

aduth Nov 20, 2018

Member

Ah, right.

@@ -4,6 +4,7 @@
*/
const HEIGHT_OFFSET = 10; // used by the arrow and a bit of empty space
const isMobileViewport = () => window.innerWidth < 782;
const isRtl = () => document.documentElement.dir === 'rtl';

This comment has been minimized.

@aduth

@youknowriad youknowriad force-pushed the update/fix-rtl-popovers branch from 13574f9 to b171fdc Nov 20, 2018

@aduth

This comment has been minimized.

Member

aduth commented Nov 20, 2018

I think the tail is still not as nicely placed as it is in LTR, but it's more a visual oddity than hugely breaking:

image

@aduth

aduth approved these changes Nov 20, 2018

@youknowriad youknowriad merged commit f91e66c into master Nov 20, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the update/fix-rtl-popovers branch Nov 20, 2018

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

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