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

Make use of logical properties consistently #1576

Closed
4 tasks done
tylersticka opened this issue Nov 8, 2021 · 3 comments · Fixed by #1595
Closed
4 tasks done

Make use of logical properties consistently #1576

tylersticka opened this issue Nov 8, 2021 · 3 comments · Fixed by #1595
Assignees
Labels
🦮 a11y Accessibility improvements 👷 dev 👷 environment Setup, build tools, configuration, etc.

Comments

@tylersticka
Copy link
Member

tylersticka commented Nov 8, 2021

Overview

Waaaaaay back in #651, we had a lot of internal discussion about logical properties, ultimately deciding to base our utilities on them. But since then, only our utilities and our Container object use logical properties: We appear to overwhelmingly be using traditional, directional property names like margin-left over equivalents like margin-inline-start.

At the time, some of our misgivings may have been a factor of confusing support. Chrome versions ≤ 86 and Safari versions ≤ 14.0 did not accurately support shorthand properties like margin-inline or padding-block. Thankfully, these issues have been resolved in more recent versions.

Another factor may be that we focused too little on this change in CSS compared to utility classes. Our spacing and padding utility classes have proven useful, with ~ 45 occurrences in our documentation and prototypes, but that's nothing compared to the 100+ occurrences of margin in our stylesheets.

Perhaps this is also a difficult change to make without some automated hand-holding. There are a few competing stylelint plugins for this, the most popular appearing a bit stale. This alternative seems a bit better maintained: https://www.npmjs.com/package/stylelint-use-logical-spec

To-dos

  • Research and confirm there are no major "gotchas" preventing us from going all-in on logical properties
  • Set up linting to enforce use of logical properties in CSS
  • Update existing CSS rules so they comply with these new rules
  • Test for regressions

Thanks @spaceninja for pointing this out during a PR review

@tylersticka tylersticka added 🦮 a11y Accessibility improvements size:5 👷 environment Setup, build tools, configuration, etc. 👷 dev labels Nov 8, 2021
@spaceninja
Copy link
Member

I was recently updating my stylelint config and considered adding https://github.com/Jordan-Hall/stylelint-use-logical-spec

In the end I opted not to because it felt like a big change. But I agree that we should definitely handle this via linting when the time comes.

@tylersticka tylersticka self-assigned this Nov 9, 2021
@tylersticka
Copy link
Member Author

tylersticka commented Nov 15, 2021

We've had some internal discussion around whether or not it makes sense to embrace logical properties in this way or not. To summarize:

Our options are:

  1. Use logical properties except for float and clear, which will mean that directional rounded corners will not work at all for ~ 77% of Safari users (assuming we launch soon, which is unlikely given our current scheduling) but they will always be in sync with other layout properties.
  2. Use logical properties except for float, clear and border-radius. Rounded corners will work everywhere, but different text directions and writing modes will cause components to fall out of sync with them (except for those that are defined via the border-radius shorthand).
  3. Replace our logical utilities with traditional direction utilities. Everything will work consistently for everyone, but it will not demonstrate our usual "future-friendly" approach to things.
  4. Do nothing and accept the inconsistency between our spacing utilities (logical properties) and everything else (direction-specific properties).

I have a simple branch running locally that adds the afore-linked Stylelint plugin and enforces the rules on everything but border-*-radius, float and clear, so basically "option 2" above. I think a valid argument can be made for option 1 as well, depending on how quickly users adopt Safari 15. But our team hasn't had bandwidth to agree on which of these options makes the most sense. So for now, I'm marking this as blocked.

@tylersticka tylersticka added the ✋ blocked Can't proceed quite yet label Nov 15, 2021
@tylersticka tylersticka removed the ✋ blocked Can't proceed quite yet label Nov 17, 2021
@tylersticka
Copy link
Member Author

I heard consensus for our team to pursue "option 2" above, so removing the blocked label from this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🦮 a11y Accessibility improvements 👷 dev 👷 environment Setup, build tools, configuration, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants