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 stylelint #601

Merged
merged 4 commits into from
Sep 10, 2018
Merged

add stylelint #601

merged 4 commits into from
Sep 10, 2018

Conversation

prydonius
Copy link
Contributor

@prydonius prydonius commented Sep 7, 2018

As per the discussion in Slack (https://kubernetes.slack.com/archives/C9D3TSUG4/p1536314108000100), this PR switches to stylelint for better compatibility with prettier.

  • replaces sass-lint
  • adds stylelint-config-prettier to ensure we don't conflict with
    prettier rules

Adnan Abdulhussein added 4 commits September 7, 2018 14:56
@@ -0,0 +1,3 @@
{
"extends": ["stylelint-config-recommended-scss", "stylelint-config-prettier"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

stylelint-config-recommended-scss is what stylelint recommends to use when using SCSS + prettier. However, this only enables the "possible errors" rules listed in https://github.com/stylelint/stylelint/blob/master/docs/user-guide/rules.md#possible-errors and none of the language feature rules. These are pretty basic rules, so I'm not sure if we'd want some of the more specific rules?

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't add more rules if there are no good reasons to do so.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @andresmgot, let's carry on with this and let's see how it goes.

BTW, remember to disable sass lint in your editor and install stylelint extension instead.

"lint": "npm-run-all lint-js lint-css"
},
"lint-staged": {
"*.{ts,tsx}": ["yarn run lint-js-fix", "prettier --write", "git add"],
"*.scss": ["yarn run lint-css-fix", "yarn run lint-css", "prettier --write", "git add"],
"*.scss": ["yarn run lint-css-fix", "prettier --write", "git add"],
Copy link
Contributor

Choose a reason for hiding this comment

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

so lint-css is not needed anymore? I thought that we wanted to add it as a guard before git add?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sass-lint didn't have autofix so there was a separate tool for that, stylelint has it built in and will do the lint and fixing in one go, similar to tslint.

r.e. running prettier before linting, I think we should do that in a separate PR, and I'm still not 100% sure we should do that. I'd like to understand more about why it was in this order in the first place.

@@ -0,0 +1,3 @@
{
"extends": ["stylelint-config-recommended-scss", "stylelint-config-prettier"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't add more rules if there are no good reasons to do so.

@prydonius prydonius merged commit 82031be into vmware-tanzu:master Sep 10, 2018
@prydonius prydonius deleted the add-stylelint branch September 10, 2018 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants