Review all the rest of the stylelint rules to see if we should add them #3

Open
jdforrester opened this Issue May 18, 2016 · 11 comments

Projects

None yet

4 participants

@jdforrester
Member

No description provided.

@Krinkle Krinkle removed the help wanted label May 25, 2016
@Volker-E
Contributor
Volker-E commented Oct 6, 2016 edited

We should add/not add (as of stylelint v7.3.1):

  • color-no-invalid-hex Yes. Disallow 2 or 5 hex char values.
  • function-blacklist No. No current use case.
  • function-linear-gradient-no-nonstandard-direction Yes. Ensure only standard-syntax in code.
  • function-max-empty-lines Yes. No extra lines in multiline property values like transform. Don't know if breaking lines isn't prohibited by other rules already, but let's make sure.
  • function-name-case: [ "lower" ] Yes. No need for cAlc()
  • function-whitelist: No. No current use case.
  • number-max-precision: Unclear. 3 seems to be a good value especially given our em sub-pixel rendering counter-actions, but might be overkill
  • time-no-imperceptible: Yes. No reason not to support this. See also https://phabricator.wikimedia.org/T77949
  • unit-blacklist: Maybe. Could make sense, currently it seems like overkill.
  • unit-case: [ "lower" ]: Yes. No reason to have 10PX somewhere.
  • unit-no-unknown: Yes, definitely.
  • unit-whitelist: No.
  • value-keyword-case: [ "lower" ]: Yes. No reason to have display: bLOck somewhere.
  • value-no-vendor-prefix: No, not currently. Might be interesting in the future, or in projects with f.e. Autoprefixer
  • value-list-max-empty-lines: Yes. 0
  • custom-property-empty-line-before/custom-property-pattern No. Makes no sense.
  • property-case: [ "lower" ]: Yes. No reason to have dISplaY: block somewhere.
  • property-no-unknown: Yes
  • property-no-vendor-prefix: No, not currently. Might be interesting in the future, or in projects with f.e. Autoprefixer
  • keyframe-declaration-no-important: No. No need.
  • declaration-block-no-shorthand-property-overrides: Yes
  • block-closing-brace-empty-line-before: [ "never" ]: Yes, failed in my tests in OOjs UI's stylelint though
  • selector-attribute-brackets-space-inside: [ "never" ]: Yes
  • selector-attribute-operator-blacklist & -whitelist: No. No current use case
  • selector-attribute-operator-space-after: [ "never" ]: Yes
  • selector-attribute-operator-space-before: [ "never" ]: Yes
  • selector-class-pattern: No. No current use case
  • selector-id-pattern: No. No current use case
  • selector-nested-pattern: No. No current use case
  • selector-no-*: No. No current use case
  • selector-pseudo-class-blacklist & -whitelist: No. No current use case
  • selector-pseudo-class-case: [ "lower" ]: Yes
  • selector-pseudo-class-no-unknown: Yes
  • selector-pseudo-class-parentheses-space-inside: [ "always" ]: Yes, now it's clear why :not() wasn't included so far, e.g. input:not([type="submit"]) should give a warning, input:not( [type="submit"] ) is wanted.
  • selector-pseudo-element-case: [ "lower" ]: Yes
  • selector-type-case: [ "lower" ]: Yes
  • selector-type-no-unknown: Yes
  • selector-max-empty-lines: No. Could be helpful for comments preceding certain selectors of a selector group
  • root-no-standard-properties: Yes
  • rule-nested-empty-line-before: No. Ambivalent. Maybe together with ignore: ["after-comment"]
  • rule-non-nested-empty-line-before: No. In various cases of our current code base used as helpers for sectioning. Not easy generalized.
  • media-feature-name-case: [ "lower" ]: Yes
  • media-feature-name-no-unknown: Yes
  • media-feature-name-no-vendor-prefix: No
  • media-feature-parentheses-space-inside: Yes
  • media-feature-range-operator-space-after & -before: Yes
  • custom-media-pattern: No. No current use case.
  • media-query-list-comma-newline-after. Ambivalent, my take would be always multi-line, as it's easier to read and is aligned to our multi selector rules.
  • at-rule-blacklist & -whitelist: No. No current use case.
  • at-rule-name-newline-after: No. Rather no, ambivalent.
  • at-rule-semicolon-newline-after: Yes
  • max-line-length: No. Causes too much trouble currently.
  • max-nesting-depth: No. Rather no right now, maybe revisit later.
  • no-empty-source: No. No real use case.
  • no-invalid-double-slash-comments: Yes. It's just affecting CSS files and usage of this less known syntax part can cause more confusion than help

Now let's do this. :)

@jdforrester
Member

These sound good to me, except for unit-no-unknown where you say both "Yes, definitely." and "No." immediately thereafter. :-) Did you mean for unit-whitelist instead?

@Volker-E
Contributor
Volker-E commented Oct 6, 2016

@jdforrester Haha, nice one. You're right, I meant unit-whitelist. Updated the comment.
Also extended the comment with the rest of the available rules.

@Volker-E Volker-E added a commit that referenced this issue Oct 7, 2016
@Volker-E Volker-E Add rest of stylelint rules
Adding the rest of stylelint rules.

Issue: #3
f5ab8c2
@Volker-E
Contributor
Volker-E commented Oct 7, 2016

I've prepared a PR.
After looking into the additions, it might be even more clear for us and for external developers, to add rules that we don't want to have included as well:

"at-rule-name-newline-after": null

Otherwise we're repeatedly having to deal with the lookup game.

@ntwb
ntwb commented Oct 8, 2016

Otherwise we're repeatedly having to deal with the lookup game.

There's a neat ESLint plugin, eslint-find-rules, I'd be cool see a stylelint version of this 😏

@Volker-E
Contributor

Nice one, @ntwb!

@jdforrester
Member

@ntwb Is there an Issue for that? Should there be, to encourage people who might want to help out?

@ntwb ntwb referenced this issue in stylelint/stylelint Oct 15, 2016
Closed

stylelint wish list #1987

@ntwb
ntwb commented Oct 15, 2016

There is now @jdforrester, or I've proposed that we should have a wish list for such ideas 😄

@ntwb
ntwb commented Oct 17, 2016

Done, nothing fancy, but added a section on the wiki home page 👍

https://github.com/stylelint/stylelint/wiki

@Volker-E Volker-E added a commit that referenced this issue Jan 11, 2017
@Volker-E Volker-E Improve `function-name-case` rule to ignore proprietary MS filters
Ignoring proprietary Microsoft filters `DXImageTransform.Microsoft.*`
in `function-name-case` rule. Follow-up to #3.
e8868e2
@Volker-E
Contributor

@edg2s To proceed with the comments on the merged pull request:

  • shorthand-property-no-redundant-values – I really think that it's acceptable for developers who write CSS to read the spec. :P I also wouldn't want to change the property again, as it's too small of a problem.
  • function-url-scheme-whitelist – don't understand what went wrong here, the rule is "function-url-scheme-whitelist": ["https", "data"] so data URIs should work. Do you have an example for me?
  • selector-attribute-brackets-space-inside – should be a follow-up pull request IMHO
  • error messages with DXImageTransform.Microsoft.* is fixed in #40
@ntwb
ntwb commented Jan 12, 2017

To add a note, there are deprecated rules in the current config See wikimedia/grunt-stylelint#32

(I suspect there will be more than just that one rule mentioned there)

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