Skip to content

chore: add baseline stylelint rules#6372

Merged
Josh-Cena merged 5 commits into
facebook:mainfrom
nschonni:stylelint-recommeneded
Jan 16, 2022
Merged

chore: add baseline stylelint rules#6372
Josh-Cena merged 5 commits into
facebook:mainfrom
nschonni:stylelint-recommeneded

Conversation

@nschonni
Copy link
Copy Markdown
Contributor

Motivation

Use the Prettier config so not to conflict with rules.
Add the Stylelint baseline recommended rules to catch additional lissues. Fixed the straight forward issues and disabled the others.
Tried with https://github.com/stylelint/stylelint-config-standard but it created much more change, so left it with the lighter baseline config

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

N/A

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

Use the Prettier config so not to conflict with rules.
Add the Stylelint baseline recommended rules to catch additional lissues
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jan 15, 2022
@netlify
Copy link
Copy Markdown

netlify Bot commented Jan 15, 2022

✔️ [V2]
Built without sensitive environment variables

🔨 Explore the source changes: 3717525

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/61e371c1d0551000075068b7

😎 Browse the preview: https://deploy-preview-6372--docusaurus-2.netlify.app

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jan 15, 2022

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟢 Performance 92
🟢 Accessibility 98
🟢 Best practices 93
🟢 SEO 100
🟢 PWA 92

Lighthouse ran on https://deploy-preview-6372--docusaurus-2.netlify.app/

Comment thread package.json Outdated
Comment thread .stylelintrc.js Outdated
Comment thread .stylelintrc.js Outdated
Comment thread .stylelintrc.js
Copy link
Copy Markdown
Collaborator

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

Okay, LGTM!

I'll figure out two things myself in subsequent PRs:

  • How to enable the standard config
  • How to make our copyright rule stricter & fixable

But this PR is ready. Thank you!

Comment thread package.json Outdated
@Josh-Cena Josh-Cena added the pr: maintenance This PR does not produce any behavior differences to end users when upgrading. label Jan 16, 2022
@Josh-Cena Josh-Cena merged commit 284c616 into facebook:main Jan 16, 2022
@nschonni nschonni deleted the stylelint-recommeneded branch January 16, 2022 01:57
@slorber
Copy link
Copy Markdown
Collaborator

slorber commented Jan 26, 2022

Does it make sense to ignore those patterns as well?

*.md
*.mdx

My IDE is complaining now ;)

/Users/sebastienlorber/.volta/bin/node /Users/sebastienlorber/Desktop/projects/docusaurus/node_modules/stylelint/bin/stylelint.js -f json --stdin-filename website/docs/static-assets.md
[{"source":"/Users/sebastienlorber/Desktop/projects/docusaurus/website/docs/static-assets.md","deprecations":[],"invalidOptionWarnings":[],"parseErrors":[],"errored":true,"warnings":[{"line":2,"column":1,"rule":"CssSyntaxError","severity":"error","text":"Unknown word (CssSyntaxError)"}]}]
/Users/sebastienlorber/Desktop/projects/docusaurus/website/docs/static-assets.md: When linting something other than CSS, you should install an appropriate syntax, e.g. "postcss-markdown", and use the "customSyntax" option

@Josh-Cena
Copy link
Copy Markdown
Collaborator

That's very weird... I didn't get it for myself. Is that from lint-staged or the lint:style command?

@slorber
Copy link
Copy Markdown
Collaborator

slorber commented Jan 26, 2022

yarn stylelint . => shows a lot of errors, apparently stylelint is able to style more than CSS files

What about excluding everything we want to skip instead of using a pattern in the CLI command?

    "lint:style": "stylelint \"**/*.css\"",

IDEs don't see this *.css filter, and this leads to this annoying banner

image

@Josh-Cena
Copy link
Copy Markdown
Collaborator

Oh... I've always just run lint:style 😅 Apparently, if you just pass it a ., your terminal will expand it for you and every single path will be passed to style lint, some of which it doesn't recognize. You should always use the lint:style command.

About your editor applying stylelint to unrelated files though, that looks very suspicious. My stylelint extension in vscode doens't do that...

@nschonni
Copy link
Copy Markdown
Contributor Author

Playing around with adding

*.*
!*.css

to the styllintignore, but it does seem like stylelint is just a little broken for it's default file globs

@slorber
Copy link
Copy Markdown
Collaborator

slorber commented Jan 26, 2022

You should always use the lint:style command

It's not me, it's my IDE :D

And I think it's perfectly legit for stylelint to lint css-in-js or md

It doesn't seem to expand to all files (not package.json for example) so maybe we could just ignore a list of extensions for now?

@Josh-Cena
Copy link
Copy Markdown
Collaborator

Yeah, that would work.

@nschonni
Copy link
Copy Markdown
Contributor Author

Opened #6476 that might resolve that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed Signed Facebook CLA pr: maintenance This PR does not produce any behavior differences to end users when upgrading.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants