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.

Sign up for GitHub

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

Jump to bottom

Stylelint fixes #1610

Merged
merged 16 commits into from
Dec 13, 2016
Merged

Stylelint fixes #1610

merged 16 commits into from
Dec 13, 2016

Conversation

shawnbot
Copy link
Contributor

@shawnbot shawnbot commented Dec 8, 2016 •
edited
Loading

Fixes our gulp stylelint task so that we can add it back in as a dependency to gulp sass and ensure that we're not breaking our own Sass style rules.

msecret
msecret reviewed Dec 8, 2016
View reviewed changes
src/stylesheets/core/_fonts.scss
@@ -1,3 +1,4 @@
/* stylelint-disable at-rule-empty-line-before */
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we disabling this rule here? Couldn't we just add a newline on line 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC, stylelint was throwing errors for every @include rule in the file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea maybe in another PR we should configure that rule, not sure it makes sense.

src/stylesheets/core/_utilities.scss
@@ -150,7 +152,7 @@
z-index: 1;
}

&.usa-current {
&.usa-current { /* stylelint-disable-line selector-no-qualifying-type */
Copy link
Contributor

Choose a reason for hiding this comment

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

This rule is not re-enabled.

But I'd also potentially prefer to just move .usa-current outside of a because I'm not sure the extra specificity is needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, it might be required because things like a:visited might override it.

src/stylesheets/core/_utilities.scss
@@ -175,7 +176,7 @@
line-height: $heading-line-height;

&:hover,
&.usa-current {
&.usa-current { /* stylelint-disable-line selector-no-qualifying-type */
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this rule ever re-enabled? Or does it need to be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

stylelint-disable-line doesn't need to be re-enabled; it only applies to the line 😀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was hoping that we could focus this PR on reducing/eliminating linting errors, then address any CSS-specific stuff separately.

msecret
msecret approved these changes Dec 8, 2016
View reviewed changes
Copy link
Contributor

@msecret msecret left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@shawnbot
Copy link
Contributor Author

shawnbot commented Dec 8, 2016

Okay @msecret, I pushed a bunch more fixes, and the two generally of note are:

  1. I removed all of the double quotes from attribute selectors, e.g. [type=submit] (because our config wants single quotes, and they're not necessary)

  2. I made a variable to work around stylelint refusing to treat this as an expression:

    @media screen and (max-width: ($nav-width - 1px)) {
//                   wants a ":" here ---^

    but this works:

     $lt-nav-width: $nav-width - 1px;
 @media screen and (max-width: $lt-nav-width) {

And, finally, I re-enabled the stylelint task as a dependency of gulp sass, so our styles get linted before being compiled! 🎉

@shawnbot shawnbot changed the title [WIP] Stylelint fixes Stylelint fixes Dec 8, 2016
juliaelman
juliaelman suggested changes Dec 13, 2016
View reviewed changes
Copy link
Contributor

@juliaelman juliaelman left a comment

Choose a reason for hiding this comment

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

One tiny change and then feel free to merge in! 🌮

config/gulp/sass.js
@@ -54,7 +54,7 @@ gulp.task('copy-vendor-sass', function (done) {
* XXX the 'stylelint' prerequisite is commented out here because
* there are currently a TON of linting errors in our SCSS files.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need the above comment anymore. 😁 🎉

@juliaelman juliaelman merged commit 8eea206 into staging Dec 13, 2016
@juliaelman juliaelman deleted the stylelint-fixes branch December 13, 2016 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juliaelman juliaelman juliaelman approved these changes

@msecret msecret msecret approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shawnbot @juliaelman @msecret