caugner commented 2 years ago

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

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

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

mustaphaturhan commented 2 years ago

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

caugner commented 2 years ago

Awesome, thank you so much! 🙏

mustaphaturhan commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 1 year ago

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 commented 1 year ago

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"

mdn / yari

Stylelint: fix current issues and prevent future issues #6455

Problem

Solution

declaration-property-value-disallowed-list

Solutions

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

Solutions

watch-submenu-setGlobal

selector-max-id

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