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 SCSS linting #27328

Merged
merged 8 commits into from Jan 3, 2019

Conversation

Projects
None yet
5 participants
@chandlerprall
Copy link
Contributor

chandlerprall commented Dec 17, 2018

Summary

closes #21251

  • Adds S(A|C)SS linting
    • explicitly specify which modules to lint via .sass-lint.yml allowing @elastic/design to update piece-by-piece (starting with x-pack's rollup and security)
    • test task alongside es/ts lint
    • jenkins:unit task alongside es/ts lint
    • will warn during dev builds this is non-normative as es/ts lint do not lint during build, I think this is better developer experience but I don't care strongly
  • Fixed dev process where a change to one module would stop watching any other modules, and a SCSS syntax error would drop all watchers

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

- [ ] This was checked for cross-browser compatibility, including a check against IE11
- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
- [ ] Documentation was added for features that require explanation or tutorials
- [ ] Unit or functional tests were updated or added to match the most common scenarios
- [ ] This was checked for keyboard-only and screenreader accessibility

For maintainers

- [ ] This was checked for breaking API changes and was labeled appropriately
- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately

chandlerprall added some commits Nov 28, 2018

@chandlerprall chandlerprall requested review from tylersmalley and snide Dec 17, 2018

@chandlerprall chandlerprall changed the title Feature/scss linting Add SCSS linting Dec 17, 2018

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Dec 17, 2018

@snide

This comment has been minimized.

Copy link
Contributor

snide commented Dec 17, 2018

Fixed dev process where a change to one module would stop watching any other modules, and a SCSS syntax error would drop all watchers

😭 THANK YOU cc @cchaos / @ryankeairns

@chandlerprall

This comment has been minimized.

Copy link
Contributor

chandlerprall commented Dec 17, 2018

CI errored as desired,

19:31:42 Running "run:sasslint" (run) task
19:31:44
19:31:44 x-pack/plugins/rollup/public/crud_app/_crud_app.scss
19:31:44 5:21 warning !important not allowed no-important
19:31:44 6:15 warning !important not allowed no-important
19:31:44
19:31:44 x-pack/plugins/security/public/views/logged_out/_logged_out.scss
19:31:44 42:3 warning Mixins should come before declarations mixins-before-declarations
19:31:44 43:3 warning Mixins should come before declarations mixins-before-declarations
19:31:44 58:41 warning Don't include leading zeros on numbers leading-zero
19:31:44 60:1 error Space expected between blocks empty-line-between-blocks
19:31:44
19:31:44 x-pack/plugins/security/public/views/login/_login.scss
19:31:44 42:3 warning Mixins should come before declarations mixins-before-declarations
19:31:44 43:3 warning Mixins should come before declarations mixins-before-declarations
19:31:44 68:41 warning Don't include leading zeros on numbers leading-zero
19:31:44 70:1 error Space expected between blocks empty-line-between-blocks

@cchaos

This comment has been minimized.

Copy link
Contributor

cchaos commented Dec 17, 2018

@chandlerprall chandlerprall requested a review from elastic/kibana-design as a code owner Dec 21, 2018

@snide

snide approved these changes Dec 21, 2018

Copy link
Contributor

snide left a comment

I tested this and it works as advertised. I went ahead and made fixes to the files being output here to verify that we'll get a passing CI.

I wish I knew of a good way to keep our linting rules up to date between the two repos, but my guess is we'll need some variation between them as well.

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Dec 21, 2018

@chandlerprall

This comment has been minimized.

Copy link
Contributor

chandlerprall commented Jan 2, 2019

jenkins test this

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jan 2, 2019

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jan 2, 2019

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jan 2, 2019

@snide

This comment has been minimized.

Copy link
Contributor

snide commented Jan 2, 2019

Anyone in @elastic/kibana-operations have a second to look at this one?

@tylersmalley
Copy link
Member

tylersmalley left a comment

LGTM

@chandlerprall chandlerprall merged commit 23037a3 into elastic:master Jan 3, 2019

2 checks passed

CLA Commit author has signed the CLA
Details
kibana-ci Build finished.
Details

@chandlerprall chandlerprall deleted the chandlerprall:feature/scss-linting branch Jan 3, 2019

chandlerprall added a commit to chandlerprall/kibana that referenced this pull request Jan 3, 2019

Add SCSS linting (elastic#27328)
* scss linting POC

* update yarn.lock

* Include sass linting alongside es/ts lint tasks

* fix linting errors

* replace unceccessary selector on rollup creation

chandlerprall added a commit that referenced this pull request Jan 3, 2019

Add SCSS linting (#27328) (#27995)
* scss linting POC

* update yarn.lock

* Include sass linting alongside es/ts lint tasks

* fix linting errors

* replace unceccessary selector on rollup creation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment