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

fix: init lint #1309

Merged
merged 15 commits into from Mar 13, 2019
Merged

fix: init lint #1309

merged 15 commits into from Mar 13, 2019

Conversation

bcullman
Copy link
Contributor

@bcullman bcullman commented Mar 13, 2019

The repo has a .sass-lint.yml file, but it was never used.

In attempting to init linting with this configuration, it I got over 5k lint errors, but worse, sass-lint was unstable, running correctly on the command line, but improperly as an extension for VSCode.

Given that linting was never enforced, and that sass-lint is unstable, I am removing the sass-lint config file (sass-lint.yml). In it's place I am initing stylelint and adding a stylelint config file (stylelintrc.yml), using a ruleset that SAP Concur has had in prior CSS repos.

Of that ruleset, I am fixing all issues that were "clean" fixes. Three remaining fixes will be handled in followup PRs. They are:

  • indentation: this will be a noisy PR. it should be handled on its own.
  • no-duplicate-selectors: requires a bit of finesse
  • declaration-property-value-blacklist: [ { '/^outline$/': 'none' }]: is considered best practice, but will require a further discussion.

Further, I have adding linting to the Travis CI run, and set up a pre-commit check so that CSS that is not linted will not get re-added to the codebase.

@joseegm
Copy link
Contributor

joseegm commented Mar 13, 2019

The Preview deployment for this PR failed.

Built with commit 954cc27

https://app.netlify.com/sites/fundamental/deploys/5c8932e8b8c89600086c8fca

@bsrdjan
Copy link
Member

bsrdjan commented Mar 13, 2019

Would you mind adding the stylelint-order plugin and initiate the properties order check as well? The sooner it starts, less work would be required later to fix.

For me personally even alphabetical convention is better than nothing and if dev team has more capacity, fixed order (twbs) or group by type would be even better.

@bsrdjan
Copy link
Member

bsrdjan commented Mar 13, 2019

Related #1298

@bcullman
Copy link
Contributor Author

@bsrdjan - great suggestion. I have limited the rules in this init PR to primarily whitespace differences and a small set of single char changes. stylelint-order is quite a bit more involved than that, so for that reason I will defer that to a future PR.

Going forward, I would like to limit complicated lint rules (non-whitespace) PRs to one-rule-per-PR

@bcullman bcullman merged commit b9c151f into master Mar 13, 2019
@bcullman bcullman deleted the fix/init-lint branch March 13, 2019 21:54
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.

None yet

6 participants