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

RTL: refactor using logical names #31210

Merged
merged 3 commits into from
Jul 29, 2020

Conversation

ffoodd
Copy link
Member

@ffoodd ffoodd commented Jun 30, 2020

Mostly a POC for now, here's a first try on using logical names for physical properties.

It's meant to:

  1. keep naming consistent between RTL and LTR (eg. prevent .float-right { float: left; } in RTL)
  2. ease the switch to custom properties one day, since naming would already be logical.

There's still things to consider here, I'm not done with SCSS yet (missing components, carousel, dropdown, popovers or tooltips for the most noticeable) and it'd require to apply those changes in our JS as well as in our docs (and to be heavily documented in the migration page).

@twbs/css-review is this a way I should follow right away?

This is highly questionnable, being consistent requires to rename for example caret-down to caret-block-end, or to use new abreviations for our utilities (.mt becomes .mbs for block-start, .etc). I tried to be consistent but there's an real amount of work here.

To do

  • update migration guide
  • update docs everywhere
  • update docs theming

Questions

What to do with:

  • RFS mixins?to tackle in RFS directly
  • Dropdowns, popovers and tooltips, relying on Popper?JS related stuff, not only renaming…
  • Carousels class names (eg .carousel-item-left) → Rename.
  • _position.scss defines fixed and sticky helpers for top and bottom. They're not impacted by RTL but will require refactor to use inset values and subvalues (eg. inset-block-start) when switching to logical properties. Should we rename classes to be consistent with the whole PR? → Do nothing.

@ffoodd ffoodd force-pushed the master-fod-rtlcss-logical-names branch from 3e622bf to 4e6d6f2 Compare June 30, 2020 12:29
@ffoodd ffoodd mentioned this pull request Jun 30, 2020
55 tasks
@ffoodd ffoodd force-pushed the master-fod-rtlcss-logical-names branch 2 times, most recently from 167a335 to e94b341 Compare July 22, 2020 14:52
@ffoodd ffoodd force-pushed the master-fod-rtlcss branch from 1a4404e to 6814906 Compare July 22, 2020 14:57
@ffoodd ffoodd force-pushed the master-fod-rtlcss-logical-names branch 3 times, most recently from 8733621 to d50b26c Compare July 22, 2020 15:09
@ffoodd
Copy link
Member Author

ffoodd commented Jul 22, 2020

@twbs/css-review I think I'm done with renaming here: please step in to review the CSS part before I continue with the documentation part. There are a few questions in the PR description, BTW :)

@ffoodd ffoodd changed the title RTLCSS: start to rename using logical names RTL: refactor using logical names Jul 23, 2020
scss/_button-group.scss Outdated Show resolved Hide resolved
@mdo
Copy link
Member

mdo commented Jul 23, 2020

One more comment while I'm here—I think our top/bottom classes can stay as-is and left/right can change to start/end. This keeps half our classes intact and reinforces a shared language across horizontal and vertical alignment.

Vertical: top, middle, bottom
Horizontal: start, center, end

Does this get us too far away from logical properties? Is it a weird bastardization? I want to keep these class names approachable.

@ffoodd
Copy link
Member Author

ffoodd commented Jul 24, 2020

@mdo yeah I've been thinking the same thing, considering the amount of changes here. The target is RTL only for v5, so logical names should only be a first step for this release.

I'll make a step back, and topics like dropdowns, tooltips and popovers will get simplified. However that'd come to us when we'll switch to logical properties: that might require to work on Popper.js at some point…

Consider this first commit as a v6 preview or something—I'll trash it while rebasing 👍

@ffoodd ffoodd force-pushed the master-fod-rtlcss-logical-names branch 5 times, most recently from f587253 to 8b67d5a Compare July 24, 2020 12:52
@ffoodd ffoodd marked this pull request as ready for review July 24, 2020 14:27
@ffoodd ffoodd requested review from a team as code owners July 24, 2020 14:27
@ffoodd
Copy link
Member Author

ffoodd commented Jul 24, 2020

@mdo I think it's ready now. To sum up:

  1. only left and right things were renamed.ms and .me for margin-left and margin-right being the most shocking changes to me (but way less than my previous attempt)
  2. docs have been revamped, I guess wording and phrasing could be better — just let me know :)
  3. a new RTL section is introduced in the migration guide — above the alpha2 since I don't think it'd be ready for alpha 2;
    • I used details and summary to sum up things a bit, should I drop it?
  4. Popper.s related questions should better be solved in main PR before rename anything here (I think there'll be more JS related topics than CSS ones)
  5. RFS is a completely different topic, I'd have to open a PR against RFS directly.

Do not forget that it can be merged in my main RTL branch and reworked later :D

@ffoodd

This comment has been minimized.

@MartijnCuppens
Copy link
Member

Not sure why and where we need additional mixins? We could use @include rfs($padding-start, padding-left);

@ffoodd

This comment has been minimized.

@ffoodd
Copy link
Member Author

ffoodd commented Jul 28, 2020

@MartijnCuppens You're right, I think there's nothing needed RFS side: we don't use its direction-related mixins in our codebase. So until the feature is asked for in RFS, I guess we're safe 👍

So the latest pain point in #30980 concerns Popper.js.

This specific PR is definetely ready to review :)

@ffoodd ffoodd force-pushed the master-fod-rtlcss branch 8 times, most recently from 48451fe to fec38cb Compare July 29, 2020 14:34
@ffoodd ffoodd force-pushed the master-fod-rtlcss-logical-names branch from 223a334 to b26bb28 Compare July 29, 2020 15:42
@ffoodd ffoodd merged commit 3a37349 into master-fod-rtlcss Jul 29, 2020
@ffoodd ffoodd deleted the master-fod-rtlcss-logical-names branch July 29, 2020 15:49
@FaridAghili
Copy link

I've used Google's Flutter for android development which supports RTL by default.
They use start and end instead of left and right as their standard without changing top and bottom to anything else.

@ffoodd ffoodd mentioned this pull request Feb 19, 2021
20 tasks
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.

4 participants