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

[core] Enable stylelint on CSS files #1184

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Dec 19, 2024

I noticed we didn't lint the .css files after my IDE reported issues opening them (using https://marketplace.visualstudio.com/items?itemName=stylelint.vscode-stylelint). It seems that we should start to lint those. There are too many rules to fix for me to make sense to do it, but I went with the simplest, quick win path:

  • lint those path
  • test if it works by fixing a few errors
  • use the current number of warnings as the current limit accepted, so the problem doesn't grow

I think next we could:

  • iterate on the rule to see the ones that make sense
  • fix them all
  • remove the warning default severity and max error count
  • do the same on https://github.com/mui/material-ui

@oliviertassinari oliviertassinari added core Infrastructure work going on behind the scenes scope: code-infra Specific to the code-infra product labels Dec 19, 2024
Comment on lines 156 to 157
--color-gray-700: oklch(92% 1.125% 264 / 80%);
--color-gray-800: oklch(92% 1.125% 264 / 80%);
--color-gray-800: oklch(93% 0.875% 264 / 85%);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicated value, seems wrong

@mui-bot
Copy link

mui-bot commented Dec 19, 2024

Netlify deploy preview

https://deploy-preview-1184--base-ui.netlify.app/

Generated by 🚫 dangerJS against c686b2c

@oliviertassinari oliviertassinari changed the title [core] Enable stylelint on css files [core] Enable stylelint on CSS files Dec 19, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 20, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 25, 2024
@michaldudak
Copy link
Member

I'd disable the following rules:

  • declaration-empty-line-before, custom-property-empty-line-before - makes long rulesets more readable
  • import-notation - the bundler understands the simple notation we use

And autofix all the remaining violations.

@oliviertassinari
Copy link
Member Author

I'd disable the following rules:

As a follow up PR though, no? First goal here is to avoid the count to increase. To note that the first one can come from https://github.com/mui/material-ui/pull/44874/files#diff-2633572b1eb62578838877abeb39ff8053af5fa47a1a731129b889db37e59610R15 if we merge this. And the second one, we could add it to the monorepo config as I guess it's relevant for everyone?

Copy link
Member

@michaldudak michaldudak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, could be a follow-up, I don't mind.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes PR: out-of-date The pull request has merge conflicts and can't be merged scope: code-infra Specific to the code-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants