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

Add ESLint rule to prevent usage of the order CSS property #61247

Open
afercia opened this issue Apr 30, 2024 · 6 comments
Open

Add ESLint rule to prevent usage of the order CSS property #61247

afercia opened this issue Apr 30, 2024 · 6 comments
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Good First Issue An issue that's suitable for someone looking to contribute for the first time [Type] Code Quality Issues or PRs that relate to code quality

Comments

@afercia
Copy link
Contributor

afercia commented Apr 30, 2024

Description

See #61241

Some components come with CSS in JS.

#61241 and #61243 aim to introduce a stylelint rule to prevent usage of the order CSS property, which is impactful for accessibility.

In the same way, it would be nice to add an ESLint rule to prevent usage of the order CSS property in any *.js file.

Unfortunately this isn't exactly my area of expertise so that I'd appreciate some guidance. I did see a similar rule added in #58130

Step-by-step reproduction instructions

  • Add an order CSS property e.g. order: 1; to any CSS-in-JS for example in this file..
  • Run npm run lint:js.
  • Observe the linter does not report any error.

Screenshots, screen recording, code snippet

No response

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@afercia afercia added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Code Quality Issues or PRs that relate to code quality labels Apr 30, 2024
@afercia
Copy link
Contributor Author

afercia commented May 2, 2024

I guess the patterns to catch would be, at least, the following ones. With and without leading spaces or tab characters:

order: 123;
    order: 123;

order: 123,
    order: 123,

order: '123',
    order: '123',


'order: 123;'
    'order: 123;'

@gziolo gziolo added the Good First Issue An issue that's suitable for someone looking to contribute for the first time label Jun 13, 2024
@ghostp13409
Copy link

Hey, I'm fairly new to open-source and web development, but I would love to give it a try

@afercia
Copy link
Contributor Author

afercia commented Jul 2, 2024

@ghostp13409 welcome. Sure, please do feel free to work on a Pull Request.

I did try to follow a previous example that added another ESLint rule, see #59022. But I'm not good at regular expressions. Also, I have no idea how that works. I did try to go through the ESLint custom rules docs and the docs for selectors but this isn't really my area of expertise. Wondering if there's any simpler way to detect the order CSS property in JS. @mirka any help would be appreciated, when you have a chance 🙇🏽‍♂️

It gets complicated because of the several ways CSS can be coded in JS. It could be a key/value pair within an object, or a string, or a concatenated string... etc.

Note the JS linter can be run in the terminal with this command: npm run lint:js -- --quiet

@mirka
Copy link
Member

mirka commented Jul 2, 2024

CSS-in-JS can either be done with strings or objects. The string case can be caught similar to #59022 without much downside, but the object case will be trickier because { order: 3 } is something that could validly appear in code unrelated to styling.

I wouldn't say this rule is particularly necessary at the moment, given that:

  • We currently have zero violations in @wordpress/components code, and we generally have more rigorous reviews on CSS that gets merged in the package. (CSS-in-JS is only used in @wordpress/components)
  • There will be increasingly fewer usages of CSS-in-JS as we migrate off of Emotion.

Might be fine to revisit once we actually encounter an order slipping through.

@Gannu456
Copy link

Gannu456 commented Nov 15, 2024

Hey @afercia , I think i can solve this issue....Can i make a pull request?

@afercia
Copy link
Contributor Author

afercia commented Nov 18, 2024

@Gannu456 thanks for looking into this. Given @mirka's previous comment, I'm not sure it is worth it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Good First Issue An issue that's suitable for someone looking to contribute for the first time [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

No branches or pull requests

5 participants