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

Fixes #118: added stylelint #125

Merged
merged 3 commits into from Nov 11, 2019
Merged

Conversation

cindyledev
Copy link
Contributor

@cindyledev cindyledev commented Nov 9, 2019

Added style lint to catch invalid css styles

How to run script:
npm run stylelint
Annotation 2019-11-09 150129

This PR successfully picks up the h1c invalid css selector in planet.css
Fixes #118

@cindyledev cindyledev changed the title added stylelint Fixes #118: added stylelint Nov 9, 2019
@Reza-Rajabi Reza-Rajabi added this to In progress/Review in Main via automation Nov 9, 2019
@Reza-Rajabi Reza-Rajabi self-requested a review November 9, 2019 20:50
Reza-Rajabi
Reza-Rajabi previously approved these changes Nov 9, 2019
package.json Show resolved Hide resolved
@cindyledev
Copy link
Contributor Author

I've hooked the stylelint script to the test script. Please review my changes.

Adding npm-run-all can be filed as another issue.

@humphd
Copy link
Contributor

humphd commented Nov 10, 2019

planet.css
 207:1  ✖  Unexpected unknown type selector "h1c"   selector-type-no-unknown

This is great, it would have caught the issue in #117.

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

This is looking great! A few questions for you.

.stylelintrc.js Outdated
"!*.css"
],
rules: {
"at-rule-no-unknown": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: why not just use the standard config of rules in https://github.com/stylelint/stylelint-config-standard? How did you arrive at this set of rules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used https://github.com/stylelint/stylelint-config-recommended , should I use the standard config instead of recommended?

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of manually defining these rules, you can install the recommended:

npm install stylelint-config-recommended --save-dev

and then use

  "extends": "stylelint-config-recommended"

And you don't/won't have to specify all these. You'd only use these rules if you wanted to override something, which we might. This is very similar to what we're doing in eslint with the Airbnb ruleset

package.json Outdated
"jest": "^24.9.0"
"jest": "^24.9.0",
"stylelint": "^11.1.1",
"stylelint-webpack-plugin": "^1.0.3"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this dep?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, this was a mistake. I thought we need both stylelint and stylelint-webpack-plugin for stylelint to work. I'll remove it

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Awesome

@Reza-Rajabi Reza-Rajabi merged commit cca2a01 into Seneca-CDOT:master Nov 11, 2019
Main automation moved this from In progress/Review to Done Nov 11, 2019
@humphd
Copy link
Contributor

humphd commented Nov 11, 2019

I'm excited to see this land, as it will help us catch bugs faster in review for subsequent PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Main
Done
Development

Successfully merging this pull request may close these issues.

Add stylelint to our automated testing
3 participants