Stylelint: fix current issues and prevent future issues

[caugner]

Problem

We have stylelint in our repository, but we don't run it systematically, so we currently have 1000+ errors:

% yarn stylelint -q '**/*.scss' | grep '✖' | wc -l
error Command failed with exit code 2.                                                                                                                                                                   
    1302

Solution

1. Verify whether the rules causing errors make sense.
2. Fix the current errors automatically (and - in a separate commit - manually if necessary).
3. Run stylelint as part of lint-staged, to avoid introducing new errors.
4. Run stylelint as part of the .github/workflows/testing.yml workflow.

Bonus: Integrate stylelint with ESLint (to avoid having to run it separately).

[mustaphaturhan]

This issue amazing for me. I can take it ✌️

[caugner]

Awesome, thank you so much! 🙏

[mustaphaturhan]

Some problems,

declaration-property-value-disallowed-list

• stylelint-config-sass-guidelines forbids usage of border: none and it cause error. (declaration-property-value-disallowed-list)

Solutions

• I don't think that disabling declaration-property-value-disallowed-list will cause any problem. So we may disable the rule.

no-descending-specificity / no-duplicate-selectors conflict

This is little bit confusing. I have to show some examples to explain this. The code block that I shared below throws no-descending-specificity error.

@media screen and (min-width: $screen-lg) {
  ul.main-menu .submenu {
    display: none;
  }

  ul.main-menu .top-level-entry-container {
    &:hover,
    &:focus-within {
      .submenu {
        display: block;
      }
    }
  }

  // This allows keyboard nav to work more predictably on desktop dropdowns.
  .open-on-focus-within {
    &:focus-within {
      .watch-submenu {
        display: flex;
      }

      .submenu {
        display: block;
      }
    }

    .submenu,
    .watch-submenu {
      display: none;
    }
  }
}

In this case, we can solve some no-descending-specificity by changing their positions. However, it causing another problem called no-duplicate-selectors. Here is an example:

@media screen and (min-width: $screen-lg) {
  .open-on-focus-within {
    .submenu,
    .watch-submenu {
      display: none;
    }
  }

  ul.main-menu .submenu {
    display: none;
  }

  // This allows keyboard nav to work more predictably on desktop dropdowns.
  .open-on-focus-within {
    &:focus-within {
      .watch-submenu {
        display: flex;
      }

      .submenu {
        display: block;
      }
    }
  }

  ul.main-menu .top-level-entry-container {
    &:hover,
    &:focus-within {
      .submenu {
        display: block;
      }
    }
  }
}

Solutions

• We might disable no-descending-specificity for some files.
• We might disable no-descending-specificity rule globally.
• We might disable no-duplicate-selectors but it is not ideal.

[caugner]

@mustaphaturhan Maybe

• To fix declaration-property-value-disallowed-list, we should be able to replace border: none with border: 0.
• For your no-descending-specificity example, the solution looks good to me.
• To fix the no-duplicate-selectors error, we should then be able to merge the two blocks that start with ul.main-menu like this:

@media screen and (min-width: $screen-lg) {
  .open-on-focus-within {
    .submenu,
    .watch-submenu {
      display: none;
    }
  }

  ul.main-menu {
    .submenu {
      display: none;
    }

    .top-level-entry-container {
      &:hover,
      &:focus-within {
        .submenu {
          display: block;
        }
    }
  }

  // This allows keyboard nav to work more predictably on desktop dropdowns.
  .open-on-focus-within {
    &:focus-within {
      .watch-submenu {
        display: flex;
      }

      .submenu {
        display: block;
      }
    }
  }
}

[mustaphaturhan]

@caugner I am here with more questions🙂

watch-submenu-setGlobal

What is this? It throws selector-class-pattern error since setGlobal isn't written in kebab-case. I will change it but can't find where it is using.

selector-max-id

Currently, stylelint allowing 0 id but it doesn't make sense for me. So I changed it to 1.

selector-max-compound-selectors and max-nesting-depth

Currently, it is expected to have no more than 3 compound selectors. And expected nesting depth should be no more than 3. There is 183 error which affected from this rules.

Should we stick with 3?

[caugner]

@mustaphaturhan Great questions!

watch-submenu-setGlobal

This last usage of this class seems to have been removed in 2db8b971f7603793b0715f2a4a008a7c7b934e5b.

(I ran git log -pS 'watch-submenu-setGlobal' to find this out.)

selector-max-id

Interesting. I get the idea of forbidding id-selectors at all (which is what 0 does, right?), but changing it to 1 sounds good to me.

selector-max-compound-selectors and max-nesting-depth

By how much would the errors decrease if we increased it to 4 or 5 respectively?

[mustaphaturhan]

@caugner wow! this is the first time I saw that command, thank you.

By how much would the errors decrease if we increased it to 4 or 5 respectively?

Result when I increase it 5

yarn stylelint -q '**/*.scss' | grep '✖' | wc -l
error Command failed with exit code 2.
26

Result when I increase it 4

yarn stylelint -q '**/*.scss' | grep '✖' | wc -l
error Command failed with exit code 2.
82

[caugner]

Result when I increase it 5

@mustaphaturhan Hm, if those 26 instances can be easily and safely resolved without side-effects, let's do that. Otherwise we can also increase it to 6 or 7, and open an issue to reduce this. At least it would prevent us from adding just another level.

[mustaphaturhan]

@caugner apparently, they can be easily and safely resolved so I set it as a 5. you may check https://github.com/mdn/yari/pull/6456.

[caugner]

FWIW Here are the number of violations per rule:

$ yarn stylelint -f json -o stylelint.json
$ cat stylelint.json | jq '.[].warnings[].rule' | sort | uniq -c | sort -rn
 529 "order/properties-alphabetical-order" -> FIXED
 411 "max-nesting-depth" -> (ignore for now)
 237 "no-descending-specificity" -> (ignore for now)
 127 "selector-max-compound-selectors" -> (ignore for now)
 110 "selector-no-qualifying-type" -> (ignore for now)
  89 "rule-empty-line-before" -> TODO fix
  34 "selector-max-id" -> TODO set to allow 1
  27 "a11y/selector-pseudo-class-focus" -> TODO fix
  25 "length-zero-no-unit" -> TODO fix
  22 "selector-pseudo-element-colon-notation" -> TODO fix
  15 "color-named" -> TODO fix
  14 "order/order" -> FIXED
  14 "a11y/media-prefers-reduced-motion" -> TODO fix (comment-ignoring one case that cannot be fixed)
  13 "no-duplicate-selectors" -> TODO fix
  11 "shorthand-property-no-redundant-values" -> TODO fix
  11 "declaration-property-value-disallowed-list" -> TODO fix
   7 "property-no-vendor-prefix" -> TODO fix
   7 "declaration-block-no-duplicate-properties" -> TODO fix
   5 "selector-class-pattern" -> TODO fix
   5 "scss/selector-no-redundant-nesting-selector" -> TODO fix
   4 "no-invalid-position-at-import-rule" -> TODO fix
   3 "no-irregular-whitespace" -> (ignore for now)
   2 "font-family-no-missing-generic-family-keyword" -> TODO fix
   2 "a11y/no-outline-none" -> TODO fix
   1 "scss/at-extend-no-missing-placeholder" -> TODO fix
   1 "function-url-quotes" -> TODO fix
   1 "function-no-unknown" -> TODO fix
   1 "custom-property-no-missing-var-function" -> TODO fix
   1 "block-no-empty" -> TODO fix

[rubiesonthesky]

It seems that @nschonni has opened few PRs for this issue but they are not linked here

• custom-property-no-missing-var-function https://github.com/mdn/yari/pull/7232
• color-named https://github.com/mdn/yari/pull/7231
• function-url-quotes https://github.com/mdn/yari/pull/7230
• block-no-empty https://github.com/mdn/yari/pull/7229

---

3 "no-irregular-whitespace" -> (ignore for now)

IMO, it could be safer to mark these with lint disable comments so that nobody fixes those by accident, if they are intented to be kept.

[caugner]

Here are updated numbers:

 461 "max-nesting-depth"
 242 "no-descending-specificity"
 176 "selector-max-compound-selectors"
 114 "selector-no-qualifying-type"
  40 "selector-max-id"
  27 "a11y/selector-pseudo-class-focus"
  22 "selector-pseudo-element-colon-notation"
  14 "a11y/media-prefers-reduced-motion"
  11 "declaration-property-value-disallowed-list"
   7 "selector-class-pattern"
   7 "property-no-vendor-prefix"
   4 "no-invalid-position-at-import-rule"
   3 "no-irregular-whitespace"
   2 "font-family-no-missing-generic-family-keyword"
   2 "a11y/no-outline-none"