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

Adds Stylelint #37

Merged
merged 2 commits into from Oct 7, 2019
Merged

Adds Stylelint #37

merged 2 commits into from Oct 7, 2019

Conversation

marcelorisse
Copy link
Contributor

@marcelorisse marcelorisse commented Oct 5, 2019

Fixes #22

Changes

  • Added linting with stylelint
  • Lint runs in precommit script

Screenshot/GIF

Checks

  • Passes linting
  • Consistent look across browsers

@jh3y
Copy link
Owner

jh3y commented Oct 7, 2019

Hey @marcelorisse 👋

This is brilliant! Thank you for contributing 👍
Do we need to add stylelint-scss as a plugin? Or does the extension handle that for us?
Are there any rules/config you think we should set?
I'm surprised there are no source changes 😅

@marcelorisse
Copy link
Contributor Author

Hello @jh3y!

This is brilliant! Thank you for contributing 👍

No problem, I'm glad I can help 🙂

Do we need to add stylelint-scss as a plugin? Or does the extension handle that for us?

No need for that, I've added stylelint's command to lint-staged so it'll run on staged files.

Are there any rules/config you think we should set?

I've used stylelint-config-recommended-scss as it's recommended by stylelint documentation:
If you use language extensions, for example @if and @extends, you can use a community config like stylelint-config-recommended-scss instead. (https://stylelint.io/#extend-a-shared-configuration). I'd say it's enough for now.

I'm surprised there are no source changes 😅

There are few errors when you run yarn stylelint "src/**/*.scss" --syntax scss but I wasn't sure if I should fix them. I can do it if you think it's a good idea.

@jh3y
Copy link
Owner

jh3y commented Oct 7, 2019

Hey @marcelorisse 👋

Do we need to add stylelint-scss as a plugin? Or does the extension handle that for us?

No need for that, I've added stylelint's command to lint-staged so it'll run on staged files.

I'm referring to stylelint-scss, the stylelint plugin 👍 I noticed that was being used in stylelint-config-recommended-scss and wondered if by using the extension, we don't need to declare it as a plugin 🤔

{
  "extends": "stylelint-config-recommended-scss",
  "plugins": [ "stylelint-scss" ]
}

However, now you mention that. It would be great to expose the linting command as an npm script inside package.json.

  "lint:whirls": "stylelint "src/**/*.scss" --syntax scss"

That way a user can manually lint from within the editor, CLI, etc. without the need to commit. Taking it a step further using react-app-rewired could allow for lint on save. I'm not sure of the benefit here though and it's certainly down to what users prefer.

As for errors. If we opt to fix them, it might be better to do that in another commit/PR 👍

@marcelorisse
Copy link
Contributor Author

Hey @marcelorisse 👋

Do we need to add stylelint-scss as a plugin? Or does the extension handle that for us?

No need for that, I've added stylelint's command to lint-staged so it'll run on staged files.

I'm referring to stylelint-scss, the stylelint plugin 👍 I noticed that was being used in stylelint-config-recommended-scss and wondered if by using the extension, we don't need to declare it as a plugin 🤔

{
  "extends": "stylelint-config-recommended-scss",
  "plugins": [ "stylelint-scss" ]
}

stylelint-config-recommended-scss already declares this plugin: https://github.com/kristerkari/stylelint-config-recommended-scss/blob/master/index.js#L5

However, now you mention that. It would be great to expose the linting command as an npm script inside package.json.

  "lint:whirls": "stylelint "src/**/*.scss" --syntax scss"

That way a user can manually lint from within the editor, CLI, etc. without the need to commit. Taking it a step further using react-app-rewired could allow for lint on save. I'm not sure of the benefit here though and it's certainly down to what users prefer.

Good idea, I can add it :)

As for errors. If we opt to fix them, it might be better to do that in another commit/PR 👍

Sounds good, should be done after merging this PR though and would be good to have an issue for that.

@jh3y
Copy link
Owner

jh3y commented Oct 7, 2019

Sounds great to me 👍

That small update to package.json and LGTM 💪

@jh3y
Copy link
Owner

jh3y commented Oct 7, 2019

Love it 🎉 This is great.

Thanks @marcelorisse for the contribution!

@jh3y jh3y merged commit 466ba0c into jh3y:master Oct 7, 2019
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.

Linting w/ Stylelint
2 participants