[tech debt] Lint Emotion CSS-in-JS #6839
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- see https://github.com/stylelint/stylelint/blob/main/docs/migration-guide/to-15.md - this removal of Prettier conflicts will save us a lot of setup for CSS-in-JS
- to Sass overrides only - we'll need a separate set of rules for CSS-in-JS later - see https://github.com/stylelint/stylelint/blob/main/docs/user-guide/rules.md#deprecated
- @see https://github.com/hudochenkov/postcss-styled-syntax + set up rule overrides as necessary to handle JS-specific issues
- in favor of simply typing `--fix` manually, or calling `yarn llint-fix` - remove lint-sass-fix entirely since we're moving away from sass
- required by parser - see https://github.com/hudochenkov/postcss-styled-syntax#known-issues
- use `mathWithUnits` and border width var instead
- use obj keys so that Emotion correctly generates the expected classNames - use conditionals in array, not in CSS generation - use separate fn for SR styles that take a custom arg
- matches Sass
Our various JS utils generally include trailing `;`s and don't need extra `;`s after them + lint a few extra trailing `}`s which stylelint also catches
- new stylelint default in v15 uses commas (complex) instead of chained `:not()`s - disable for Sass - wasn't previously linted
- valid usage in this case, but in general we should lint for it
- `except` isn't handling JS interpolation well, so change it to `ignore` instead
- Sass will be going away soon and is irrelevant - CSS-in-JS has one deprecation warning (extra semicolons), but it's pretty dang useful, so I think we should not warn on it
|
@elastic/eui-team Would super appreciate a review on this whenever any of y'all have a sec! It's a lot of lines but most of it is either just I'll be doing more Emotion work this week after this so being able to auto-lint that work would be 💯 |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6839/ |
1Copenut
approved these changes
Jun 14, 2023
- I'm sure this will come up in the future otherwise
|
Thanks Trevor! I realized that the |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6839/ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
closes #6477
I strongly recommend following along by commit, but here's the TL;DR:
stylelintto v15block-no-emptyfalse positive that was otherwise cropping up frequently*.style.tsetc files now caught by our linter//comments in CSS-in-JS - we must use CSS-allowed/* */comments only. The parser will error otherwiseQA
yarn lintpasses*.styles.tsfile and break linting on purpose (e.g. add a semicolon to the end of a${logicalCSS()};line)yarn lint-css-in-jsand confirm it flags the new erroryarn lint-fixand confirm it auto-fixes the errorGeneral checklist
[ ] A changelog entry exists and is marked appropriatelyN/A, should be dev-only / not have any user-facing changes