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

Install CSS linting #507

Merged
merged 7 commits into from
Mar 4, 2020
Merged

Install CSS linting #507

merged 7 commits into from
Mar 4, 2020

Conversation

dromo77
Copy link
Contributor

@dromo77 dromo77 commented Feb 28, 2020

Installs stylelint-config-cloudfour, 🤞 that I installed it correctly. Changes to the config will be addressed in a separate PR.

@tylersticka
Copy link
Member

Hmm, I'm realizing that repo doesn't go into quite as much detail as our eslint one in terms of setting up scripts to lint styles.

@spaceninja Do you think this is a good enough first step to merge and build on later or should we update this PR with more fancy-ness?

@spaceninja
Copy link
Member

spaceninja commented Feb 28, 2020

Great question! By adding a stylelintrc, you've enabled editors to understand that you want to run Stylelint. So this PR alone would enable anyone with Stylelint installed in VS Code to benefit from in-editor linting.

However, it doesn't easily enable anyone to run linting from the command line or as part of CI or deploy steps. A pattern we've followed on several other other projects is adding a few scripts to package.json:

  • a lint:css command that runs Stylelint: stylelint '**/*.{scss}'
  • a lint:js command that runs ESLint: eslint '**/*.{js}'
  • a lint command that runs both: npm run lint:js && npm run lint:css

If this repo commits the CSS files, you'll want to lint the compiled CSS files. If not, then you'll want to lint the SCSS files.

If this repo has any sort of CI, a hook could be added to the CI config to run linting.

Eventually, when we're deploying this to npm, we'll want to run linting as part of a prepublishOnly command.

Another thing this PR doesn't touch (but is out of scope for this initial PR) is integrating Prettier into Stylelint. I just realized that would be a good blog post, talking about how to integrate Prettier into ESLint and Stylelint configs.

The reason our documentation in the Stylelint config doesn't mention this is because it dates back to when I was first configuring this at my last job and was a lot more timid around suggesting config changes. The readme file for the Stylelint config definitely needs a note on adding scripts to run it.

Action Items for this PR:

  • Add the lint scripts, configured to run on the files we care about linting.

Future PRs:

  • When we deploy to npm, we should run linting as part of prepublishOnly
  • If we add CI, we should run linting

Changes for our Lint configs:

@spaceninja
Copy link
Member

@calebeby Any other best practices around running linting?

@calebeby
Copy link
Member

For JS the linting is being run in GH Actions, the same could be done for this

@tylersticka
Copy link
Member

@dromo77 Do you think we can complete the action item for this PR that @spaceninja highlighted above before your end-of-day?

@dromo77
Copy link
Contributor Author

dromo77 commented Feb 28, 2020

@tylersticka I just added the lint scripts @spaceninja mentioned, I'm just not sure if I did correctly 🤔

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
Co-Authored-By: Scott Vandehey <scott@cloudfour.com>
@tylersticka tylersticka added ✍️ help wanted Extra attention is needed 👷 environment Setup, build tools, configuration, etc. labels Mar 2, 2020
spaceninja
spaceninja previously approved these changes Mar 2, 2020
Copy link
Member

@spaceninja spaceninja left a comment

Choose a reason for hiding this comment

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

Approved for adding linting. Either as part of this PR or as a followup, there are a number of CSS lint violations now.

@calebeby calebeby changed the title Install linting Install CSS linting Mar 3, 2020
@tylersticka
Copy link
Member

I just pushed a few updates:

  • I added the --fix flag to lint:css so its behavior is consistent with lint:js.
  • The previous lint:css script is now check-lint:css.
  • The previous check-lint script is now check-lint:js.
  • The new check-lint script now calls both check-lint tasks.
  • I ran lint:css so the stylesheets so far pass tests.
  • I resolved some merge conflicts with the latest v-next.

Copy link
Member

@calebeby calebeby left a comment

Choose a reason for hiding this comment

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

I didn't run it locally but I saw that CI ran stylelint successfully so LGTM 👍

@tylersticka tylersticka merged commit 6bbd99f into v-next Mar 4, 2020
@tylersticka tylersticka deleted the linting-config branch March 4, 2020 01:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👷 environment Setup, build tools, configuration, etc. ✍️ help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants