[Emotion] Add eslint rule for CSS logical properties #6125

cee-chen · 2022-08-11T02:29:53Z

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 --fix option 😎 (although generally we prefer to use our logicalCSS utils instead of the plain logical CSS)

Checklist

Checked in mobile
Checked in Chrome, Safari, Edge, and Firefox

~~- [ ] 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~~

kibanamachine · 2022-08-11T02:57:54Z

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

thompsongl · 2022-08-11T13:56:51Z

do wonder if we could make a homebrew eslint rule that checks the contents of css string literals for the properties in logicals.ts

Seems like a neat idea to me!

cee-chen · 2022-08-12T20:22:14Z

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! 🎉

kibanamachine · 2022-08-12T20:47:15Z

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

kibanamachine · 2022-08-16T17:26:34Z

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

- so that both script/ JS and src/ TS can use it

- 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

cee-chen · 2022-08-17T02:01:19Z

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 🤪

kibanamachine · 2022-08-17T02:21:22Z

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``

cee-chen · 2022-08-18T18:17:41Z

scripts/eslint-plugin/css_logical_properties.js

+      TemplateLiteral(node) {
+        const templateContents = node.quasis || [];
+        templateContents.forEach((quasi) => {
+          const stringLiteral = quasi?.value?.raw;


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 css templates, anywhere

lint all template literals but only within **/*.styles.ts or src/global_styling files

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

kibanamachine · 2022-08-18T18:43:25Z

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

…operties

kibanamachine · 2022-08-18T19:32:50Z

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

chandlerprall

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.

kibanamachine · 2022-08-22T21:07:06Z

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

cee-chen added the skip-changelog label Aug 11, 2022

cee-chen requested a review from chandlerprall August 12, 2022 20:22

cee-chen mentioned this pull request Aug 16, 2022

[Emotion] Convert global className utilities #6124

Merged

7 tasks

cee-chen force-pushed the more-logical-properties branch from e5879b2 to b9f52cf Compare August 16, 2022 16:58

cee-chen changed the title ~~[Emotion] Convert some missed properties to logical properties~~ [Emotion] Add eslint rule for CSS logical properties Aug 16, 2022

cee-chen added 2 commits August 16, 2022 17:54

Add custom Emotion css eslint plugin for non-logical properties

01282cd

Convert logicals map to JSON

c7ba276

- so that both script/ JS and src/ TS can use it

cee-chen force-pushed the more-logical-properties branch 2 times, most recently from 5d1fbb2 to 12f2324 Compare August 17, 2022 01:40

cee-chen added 16 commits August 16, 2022 18:53

Fix datagrid docs TS JSON imports typing

c7f7ef8

- throwing as a result of our new `resolveJsonModule: true`

Convert old static map to use imported logicals map

fed47ec

- importing the map directly with a regex for newlines produces less false positives for non-logical properties

Remove inset property from map

6f39b2c

- it's already a new logical property and doesn't need to be converted

Re-add catch for text-align logical values

8974331

+ add separate message for values vs properties - this might get used more for other future properties

Add ability to --fix eslint rule

847c4e7

- via the maps we've set up

Add logic for determining exact css property location

ab260b4

- vs highlighting the entire css`` block - yay for even more gnarly regex utils

Convert global HTML height property

d484f9f

Typography

b3f90a9

EuiScreenReaderOnly

ba73280

EuiAccordion

271eec4

EuiBeacon

919ff5f

EuiButton

ef7fdb3

EuiCallOut

1dc71a8

EuiComment

aadc830

EuiExpression

210719e

cee-chen added 9 commits August 16, 2022 18:53

EuiImage - use euiBreakpoint mixin

6100ce6

- instead of manual media query

EuiImage inset

3f479e7

Loading components

b3fb022

note: loading_logo.styles already had so many static logical properties i just continued using them for in-file consistency

EuiPageTemplate

0ddd56e

EuiProgress

71bce5c

EuiToast

5a95771

EuiPanel

f4c3fd5

Fix misc src-doc usages

72eb74f

Fix scroll documentation usages + simplify snippets

a0b5625

cee-chen force-pushed the more-logical-properties branch from 12f2324 to a0b5625 Compare August 17, 2022 01:53

[PR feedback] lint *all* template literals, not just css``

172b3b8

- since we have several Emotion utilities that pass back raw ``s instead of css``

cee-chen force-pushed the more-logical-properties branch from 902b0a9 to cfb35a3 Compare August 18, 2022 18:14

cee-chen commented Aug 18, 2022

View reviewed changes

cee-chen added 5 commits August 18, 2022 11:59

Fix src styles missing logical properties

f69ec7c

Fix over-aggressive src-docs linting by removing template literals

2cb2327

Fix over-aggressive src-docs linting by disabling rule

f388038

Fix valid src-docs catches

b4a1942

- although debatable how useful they are if we're moving away from Sass

Add dev documentation on how to disable the lint rule for specific pr…

abaeaa4

…operties

cee-chen force-pushed the more-logical-properties branch from cfb35a3 to abaeaa4 Compare August 18, 2022 19:04

chandlerprall approved these changes Aug 22, 2022

View reviewed changes

Merge branch 'main' into more-logical-properties

93b5d74

cee-chen enabled auto-merge (squash) August 22, 2022 20:39

cee-chen merged commit 4c285cd into elastic:main Aug 22, 2022

cee-chen deleted the more-logical-properties branch August 22, 2022 21:33

chandlerprall mentioned this pull request Aug 24, 2022

Importing from versions 65.0.0 and 65.0.1 do not work #6171

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Emotion] Add eslint rule for CSS logical properties #6125

[Emotion] Add eslint rule for CSS logical properties #6125

cee-chen commented Aug 11, 2022 •

edited

Loading

kibanamachine commented Aug 11, 2022

thompsongl commented Aug 11, 2022

cee-chen commented Aug 12, 2022 •

edited

Loading

kibanamachine commented Aug 12, 2022

kibanamachine commented Aug 16, 2022

cee-chen commented Aug 17, 2022

kibanamachine commented Aug 17, 2022

cee-chen Aug 18, 2022 •

edited

Loading

cee-chen Aug 18, 2022 •

edited

Loading

kibanamachine commented Aug 18, 2022

kibanamachine commented Aug 18, 2022

chandlerprall left a comment

kibanamachine commented Aug 22, 2022

[Emotion] Add eslint rule for CSS logical properties #6125

[Emotion] Add eslint rule for CSS logical properties #6125

Conversation

cee-chen commented Aug 11, 2022 • edited Loading

Summary

Checklist

kibanamachine commented Aug 11, 2022

thompsongl commented Aug 11, 2022

cee-chen commented Aug 12, 2022 • edited Loading

kibanamachine commented Aug 12, 2022

kibanamachine commented Aug 16, 2022

cee-chen commented Aug 17, 2022

kibanamachine commented Aug 17, 2022

cee-chen Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

cee-chen Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

Control (random unrelated PR from 3 days ago without this lint rule)

Linting just css templates

Linting all template literals:

Conclusion: no significant perf impact

kibanamachine commented Aug 18, 2022

kibanamachine commented Aug 18, 2022

chandlerprall left a comment

Choose a reason for hiding this comment

kibanamachine commented Aug 22, 2022

cee-chen commented Aug 11, 2022 •

edited

Loading

cee-chen commented Aug 12, 2022 •

edited

Loading

cee-chen Aug 18, 2022 •

edited

Loading

cee-chen Aug 18, 2022 •

edited

Loading