[Emotion] Add eslint rule for CSS logical properties#6125
[Emotion] Add eslint rule for CSS logical properties#6125cee-chen merged 34 commits intoelastic:mainfrom
Conversation
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6125/ |
Seems like a neat idea to me! |
|
I spent my spacetime Friday figuring out how to write an eslint rule and was able to do so! hooray! 438b861 It could maybe use a little more refinement in flagging the exact location of the property instead of the entire CSS string, but it got a little gnarly due to the string literal usage, so maybe another day 😬 The bad news is that CI will now fail because there's a bunch of property fixes I did in #6124 and not this PR, so now #6124 needs to get in first 😅 Overall though very excited for this plugin and not having to try to remember it in my own conversions or search for it in code review! 🎉 |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6125/ |
e5879b2 to
b9f52cf
Compare
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6125/ |
- so that both script/ JS and src/ TS can use it
5d1fbb2 to
12f2324
Compare
- throwing as a result of our new `resolveJsonModule: true`
- importing the map directly with a regex for newlines produces less false positives for non-logical properties
- it's already a new logical property and doesn't need to be converted
+ add separate message for values vs properties - this might get used more for other future properties
- switching to a exec allows us to get the start index of the match, so that we can more accurately return the node location and also use a fixer - (opinionated) using a single regex vs multiple loops also simplifes logic somewhat, even if the regex itself is annoying - use regex capture groups for readability
- via the maps we've set up
- vs highlighting the entire css`` block - yay for even more gnarly regex utils
- instead of manual media query
note: loading_logo.styles already had so many static logical properties i just continued using them for in-file consistency
12f2324 to
a0b5625
Compare
|
Alrighty, this PR now has a way fancier eslint rule 😅 PR description has been updated with screenshots! I very strongly recommend following along by commit if possible, regexes are a gnarly beast, even more so when using regex utils sourced from stack overflow 🤪 |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6125/ |
- since we have several Emotion utilities that pass back raw ``s instead of css``
902b0a9 to
cfb35a3
Compare
| TemplateLiteral(node) { | ||
| const templateContents = node.quasis || []; | ||
| templateContents.forEach((quasi) => { | ||
| const stringLiteral = quasi?.value?.raw; |
There was a problem hiding this comment.
Quick comment/note on 172b3b8 and the change from linting css literals to all `` literals:
If this more aggressive lint rule becomes an issue in the future (either from a dev experience or performance POV), we should consider reverting the change and imposing the following limits for it:
- lint all
csstemplates, anywhere - lint all template literals but only within
**/*.styles.tsorsrc/global_stylingfiles
There was a problem hiding this comment.
Also, per Chandler's suggestion to benchmark performance impact on CI/yarn lint runs:
Control (random unrelated PR from 3 days ago without this lint rule)
Linting just css templates
52min
Linting all template literals:
51min
Conclusion: no significant perf impact
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6125/ |
- although debatable how useful they are if we're moving away from Sass
cfb35a3 to
abaeaa4
Compare
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6125/ |
chandlerprall
left a comment
There was a problem hiding this comment.
Exciting! Thanks for taking the time to make this work with --fix as well. I can't find a case where this breaks or otherwise doesn't perform as expected.
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6125/ |
Summary
This PR adds a new local eslint rule that looks for CSS properties that should instead use logical properties and flags them.
Because @chandlerprall is the bomb, it also includes an auto
--fixoption 😎 (although generally we prefer to use ourlogicalCSSutils instead of the plain logical CSS)Checklist
- [ ] Checked in both light and dark modes- [ ] Props have proper autodocs and playground toggles- [ ] Added documentation- [ ] Checked Code Sandbox works for any docs examples- [ ] Added or updated jest and cypress tests- [ ] Checked for breaking changes and labeled appropriately- [ ] Checked for accessibility including keyboard-only and screenreader modes- [ ] Updated the Figma library counterpart- [ ] A changelog entry exists and is marked appropriately