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

Stylelint: Add no-descending-specificity and fix offenders #1119

Open
jdlrobson opened this Issue Sep 15, 2018 · 8 comments

Comments

Projects
None yet
5 participants
@jdlrobson
Collaborator

jdlrobson commented Sep 15, 2018

From the stylelint docs:
"Source order is important in CSS, and when two selectors have the same specificity, the one that occurs last will take priority. However, the situation is different when one of the selectors has a higher specificity. In that case, source order does not matter: the selector with higher specificity will win out even if it comes first.

The clashes of these two mechanisms for prioritization, source order and specificity, can cause some confusion when reading stylesheets. If a selector with higher specificity comes before the selector it overrides, we have to think harder to understand it, because it violates the source order expectation. "

Stylelint (which can be run by using npm install && npm run lint:fix) can detect these if configured with the following:
"no-descending-specificity": true,

Note: there are currently lots of offenders for this rule, so it may be worth waiting until master.less is more manageable (#1092) before attempting this

Checklist

  • Update .stylelintrc.json
  • fix any offenders detected via npm run lint:fix
@jdlrobson

This comment has been minimized.

Show comment
Hide comment
@jdlrobson

jdlrobson Sep 28, 2018

Collaborator

Better to have sooner rather than later... will work on this if nobody else does.

Collaborator

jdlrobson commented Sep 28, 2018

Better to have sooner rather than later... will work on this if nobody else does.

@jdlrobson

This comment has been minimized.

Show comment
Hide comment
@jdlrobson

jdlrobson Oct 5, 2018

Collaborator

This ones also going to be a bit tricky. There are lots of offenders :-S

Collaborator

jdlrobson commented Oct 5, 2018

This ones also going to be a bit tricky. There are lots of offenders :-S

@thefifthisa

This comment has been minimized.

Show comment
Hide comment
@thefifthisa

thefifthisa Oct 13, 2018

Contributor

Hey @jdlrobson! I'd like to help out with this if you still need it.

Contributor

thefifthisa commented Oct 13, 2018

Hey @jdlrobson! I'd like to help out with this if you still need it.

@jdlrobson

This comment has been minimized.

Show comment
Hide comment
@jdlrobson

jdlrobson Oct 13, 2018

Collaborator

We do! Go for it @thefifthisa !

Collaborator

jdlrobson commented Oct 13, 2018

We do! Go for it @thefifthisa !

@thefifthisa

This comment has been minimized.

Show comment
Hide comment
@thefifthisa

thefifthisa Oct 13, 2018

Contributor

@jdlrobson Great, thanks! Can I know which ones you've already done so I know where to start?

Contributor

thefifthisa commented Oct 13, 2018

@jdlrobson Great, thanks! Can I know which ones you've already done so I know where to start?

@jdlrobson

This comment has been minimized.

Show comment
Hide comment
@jdlrobson

jdlrobson Oct 15, 2018

Collaborator

@thefifthisa I'd recommend to start with adding the rule to the existing stylelint config file and making sure you can see the errors locally when you run

npm install
npm run lint:fix

screen shot 2018-10-15 at 4 46 27 pm

I'm seeing lots of errors when I add this rule.

Let's keep the first pull request small and reasonably safe from merge conflicts! As a general rule, I'd recommend disabling the rule at the top of large files which have more than 6 errors [1]. (e.g. static/css/components/header-bar.less and static/css/legacy.less) and focusing on the files which have less problems.

The goal with this linting is to slowly chip away at the CSS and improve it so don't take too much on at a time - focus on the files with 1-6 issues! Otherwise, it will make it harder for you to write and others to review.

Does this make sense?

Let me know if any more guidance needed!

[1] Adding the following line at the top of the file will stop the rule from applying there!

/* stylelint-disable no-descending-specificity */
Collaborator

jdlrobson commented Oct 15, 2018

@thefifthisa I'd recommend to start with adding the rule to the existing stylelint config file and making sure you can see the errors locally when you run

npm install
npm run lint:fix

screen shot 2018-10-15 at 4 46 27 pm

I'm seeing lots of errors when I add this rule.

Let's keep the first pull request small and reasonably safe from merge conflicts! As a general rule, I'd recommend disabling the rule at the top of large files which have more than 6 errors [1]. (e.g. static/css/components/header-bar.less and static/css/legacy.less) and focusing on the files which have less problems.

The goal with this linting is to slowly chip away at the CSS and improve it so don't take too much on at a time - focus on the files with 1-6 issues! Otherwise, it will make it harder for you to write and others to review.

Does this make sense?

Let me know if any more guidance needed!

[1] Adding the following line at the top of the file will stop the rule from applying there!

/* stylelint-disable no-descending-specificity */
@thefifthisa

This comment has been minimized.

Show comment
Hide comment
@thefifthisa

thefifthisa Oct 16, 2018

Contributor

@jdlrobson Got it, just submitted a PR!

Contributor

thefifthisa commented Oct 16, 2018

@jdlrobson Got it, just submitted a PR!

@JonathanArias510

This comment has been minimized.

Show comment
Hide comment
@JonathanArias510

JonathanArias510 Oct 18, 2018

I just want a t-shirt

JonathanArias510 commented Oct 18, 2018

I just want a t-shirt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment