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

[WIP][rtl][css] Fix button margins for RTL langs #3974

Merged
merged 4 commits into from
Sep 2, 2020

Conversation

nyurik
Copy link
Contributor

@nyurik nyurik commented Aug 29, 2020

Summary

When showing a button in RTL, margin-left/right shows up incorrectly. The needed CSS seems to be margin-inline-start/end. Partially relates to #3960

Those values do not exist in IE, so not sure if we can introduce these or not just yet. I tested these changes with a search bar's filters and EuiHeaderLink with icons.

english current RTL desired RTL
image image image

The above changes can be achieved with this CSS override.

.euiButtonContent > * + * {
    margin-left: unset;
    -webkit-margin-start: 8px;
    margin-inline-start: 8px; }

.euiButtonContent--iconRight > * + * {
    margin-left: unset;
    margin-right: unset;
    -webkit-margin-start: 0;
    margin-inline-start: 0;
    -webkit-margin-end: 8px;
    margin-inline-end: 8px; }

.euiFacetButton__content > * + * {
    margin-left: unset;
    -webkit-margin-start: 8px;
    margin-inline-start: 8px; }

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs
  • Added documentation
  • Checked Code Sandbox works for the any docs examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@nyurik nyurik requested a review from cchaos August 29, 2020 05:29
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3974/

@cchaos
Copy link
Contributor

cchaos commented Aug 31, 2020

Really interesting find! I was curious to see if it had any effect on the current layout and overlaid the old on top of the new, and there were no effects.

Here's a screenshot for those who want to see the LTR vs RTL.

Screen Shot 2020-08-31 at 18 40 28 PM

It of course doesn't change the actual text which is dependent on the source to be served in the correct direction.

My one concern was for consumers or other components that were possibly altering this margin and using the original syntax. So I faked changing the margin-left: 0 in the dev tools and..

It does seem to override the new property. (Expand for screenshot) Screen Shot 2020-08-31 at 18 41 47 PM
But not properly the correct side when in RTL mode (Expand for screenshot) Screen Shot 2020-08-31 at 18 45 38 PM

So, the custom overrides will not be negatively affected unless in RTL mode which isn't be tested much anyway right now.


@snide I don't see much problem with this change based on my explorations above. But I'd like to get your quick reaction to this change and see if your brain can find any other thing we should test first.

@snide
Copy link
Contributor

snide commented Sep 1, 2020

I ran through it a bit locally. I'll have to give you credit @nyurik, it's pretty rare to run into some CSS code that either @cchaos or I haven't seen before. From all the reading I can see, this seems to be a good solution to the problem. My guess is that we have other parts of the codebase that this might be useful in.

Can I use shows this as safe for everything but IE11, which we've recently dropped. I think this looks ok to merge. It'll need a changelog. I didn't get a chance to test Safari though, but as long as that works LGTM.

@nyurik
Copy link
Contributor Author

nyurik commented Sep 1, 2020

Thanks @cchaos and @snide for such thorough review! I wonder why the tests failed though, but perhaps if i ask jenkins test this it may resolve itself. I sadly cannot test on Safari (Linux doesn't run it). And thanks for dropping IE, RIP! :)

There are some other issues with RTL I'm exploring - e.g. somehow in RTL the screen instantly becomes horizontally scroll-able with lots of empty space on the left. (try it here and change lang to Hebrew in the upper right corner). But that's for the future.

When showing a button in RTL, margin-left/right shows up
incorrectly. The needed CSS seems to be `margin-inline-start/end`.

Those values do not exist in IE, so not sure if we can introduce these or not just yet.
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3974/

@nyurik nyurik marked this pull request as ready for review September 1, 2020 05:35
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

👍 I browser checked (on Mac) Chrome, Firefox, Safari, and Edge. They all behave the same. 🎉

As @snide mentioned, this is probably just a start. We'll eventually need to update more components that combine icon and text but at least we now know what to do 😉

Thanks @nyurik !!!

CHANGELOG.md Outdated Show resolved Hide resolved
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3974/

@nyurik nyurik merged commit b82e8e4 into master Sep 2, 2020
@nyurik nyurik deleted the nyurik/fix-rtl-margins branch September 2, 2020 02:53
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