Issue 29541680: Issue 5109 - Create stylelintrc for websites and ui modules (Closed)
|
Created:
Sept. 11, 2017, 9:21 a.m. by ire Modified:
Dec. 14, 2017, 2:49 p.m. CC:
kzar Base URL:
https://hg.adblockplus.org/codingtools Visibility:
Public. |
DescriptionIssue 5109 - Create stylelintrc for websites and ui modules
Based on codingtools
COLLABORATOR=manvel@adblockplus.org
COLLABORATOR=thomas@adblockplus.org
COLLABORATOR=julian@adblockplus.org
Patch Set 1 #
Total comments: 39
Patch Set 2 : Fixed typos, removed unit-no-unkown, reordered brace rules #
Total comments: 2
Patch Set 3 : Add max-line-length, update README with usage information #
Total comments: 5
Patch Set 4 : Fix typo in README #
Total comments: 1
Patch Set 5 : Change block-opening-brace-space-before to always-single-line #
Total comments: 19
Patch Set 6 : Add new rules #Patch Set 7 : Remove comments with no rules, allow no-empty line after-comment and inside-block #
Total comments: 7
Patch Set 8 : Order: opening before > opening after > closing before > closing after #
Total comments: 11
Patch Set 9 : Remove rules about inner spacing between braces, reword rule descriptions #Patch Set 10 : Change rulen description to 'short' instead of '3' character hex notation #Patch Set 11 : Fix trailing whitespace errors #
MessagesTotal messages: 30
Ready for review. https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... File stylelint-config-eyeo/index.js (right): https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/index.js:38: // CSS color values should be specified in hexadecimal where possible I couldn't find stylelint rules for these. Not sure if I should leave the comments anyway? https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/index.js:42: // CSS rule declaration order should follow the WordPress CSS Coding Standards The declaration order is no longer part of stylelint. If we really want this to be part of our stylelint, I think we can use this plugin (https://www.npmjs.com/package/stylelint-order). https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/index.js:52: "indentation": 2, This isn't actually documented in https://adblockplus.org/en/coding-style, but is a rule we seem to follow
https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... File stylelint-config-eyeo/README.md (right): https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/README.md:3: A [Styelint](https://stylelint.io) configuration that checks for compliance with the [Adblock Plus coding style guide](https://adblockplus.org/coding-style#javascript) which is used for all eyeo projects. I don't see how this affects our JavaScript code so I assume you wanted to deep-link to https://adblockplus.org/coding-style#html-css instead. https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/README.md:3: A [Styelint](https://stylelint.io) configuration that checks for compliance with the [Adblock Plus coding style guide](https://adblockplus.org/coding-style#javascript) which is used for all eyeo projects. Typo: Replace "Styelint" with "Stylelint" https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/README.md:7: npm install -g eslint stylelint-config-eyeo I assume you meant to write "stylelint" instead of "eslint". At least it seems odd to use ESLint for linting CSS files. https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... File stylelint-config-eyeo/index.js (right): https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/index.js:29: "block-closing-brace-space-before": "always-single-line", Which part of the coding style does this rule refer to? Same applies to "block-opening-brace-space-after" https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/index.js:31: "block-opening-brace-newline-before": "always-multi-line", Allowing single-line rules here sounds incompatible with the statement "Opening braces go on their own line". If it indeed turns out that we don't allow single-line rules, it would furthermore render rules with the value "always-single-line" redundant. https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/index.js:38: // CSS color values should be specified in hexadecimal where possible On 2017/09/11 09:26:38, ire wrote: > I couldn't find stylelint rules for these. Not sure if I should leave the > comments anyway? It's more of a guideline than a strict rule. Some more info on that (from what I remember): - We use `rgba()` instead of `#RRGGBBAA` for compatibility. - Under some circumstances using `hsl()` or even `rgb()` could be more appropriate than a hex value so prohibiting those outright is likely not a good idea. Therefore `"color-named": "never"` should be sufficient for now. https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/index.js:42: // CSS rule declaration order should follow the WordPress CSS Coding Standards On 2017/09/11 09:26:38, ire wrote: > The declaration order is no longer part of stylelint. If we really want this to > be part of our stylelint, I think we can use this plugin > (https://www.npmjs.com/package/stylelint-order). Given that this rule can be quite confusing for some people it may be worth adding unless we want to open the discussion about declaraction order again. https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/index.js:45: "unit-no-unknown": true, This is not what "should specify units where possible" refers to. It's about cases like `width: 0;` where no unit is provided. I don't think there's a strong reason to keep it though, in case we want to abandon it. https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/index.js:52: "indentation": 2, On 2017/09/11 09:26:38, ire wrote: > This isn't actually documented in https://adblockplus.org/en/coding-style, but > is a rule we seem to follow Under "General" we link to https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style... which states "Two spaces per logic level, or four spaces in Python code." https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... File stylelint-config-eyeo/package.json (right): https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/package.json:26: "stylelint-config-recommended": "^1.0.0" What is this dependency for? We don't have a build process that depends on it, as far as I see.
Thanks Thomas! I had just copied most of the README/package.json from the eslint-config project and overlooked some things :/ Changes made, comments below. https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... File stylelint-config-eyeo/README.md (right): https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/README.md:3: A [Styelint](https://stylelint.io) configuration that checks for compliance with the [Adblock Plus coding style guide](https://adblockplus.org/coding-style#javascript) which is used for all eyeo projects. On 2017/09/13 18:12:52, Thomas Greiner wrote: > Typo: Replace "Styelint" with "Stylelint" Done. https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/README.md:3: A [Styelint](https://stylelint.io) configuration that checks for compliance with the [Adblock Plus coding style guide](https://adblockplus.org/coding-style#javascript) which is used for all eyeo projects. On 2017/09/13 18:12:52, Thomas Greiner wrote: > I don't see how this affects our JavaScript code so I assume you wanted to > deep-link to https://adblockplus.org/coding-style#html-css instead. Done. https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/README.md:7: npm install -g eslint stylelint-config-eyeo On 2017/09/13 18:12:52, Thomas Greiner wrote: > I assume you meant to write "stylelint" instead of "eslint". At least it seems > odd to use ESLint for linting CSS files. Done. https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... File stylelint-config-eyeo/index.js (right): https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/index.js:29: "block-closing-brace-space-before": "always-single-line", On 2017/09/13 18:12:52, Thomas Greiner wrote: > Which part of the coding style does this rule refer to? > > Same applies to "block-opening-brace-space-after" We link to the Google HTML/CSS Style guide, which has rules on having a space before opening braces - https://google.github.io/styleguide/htmlcssguide.html#Declaration_Block_Separ... https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/index.js:31: "block-opening-brace-newline-before": "always-multi-line", On 2017/09/13 18:12:52, Thomas Greiner wrote: > Allowing single-line rules here sounds incompatible with the statement "Opening > braces go on their own line". > > If it indeed turns out that we don't allow single-line rules, it would > furthermore render rules with the value "always-single-line" redundant. I think I assumed that single-line rules were okay when it's just one declaration. I can't seem to find any rule for/against this though, is this documented somewhere? (If not, can we decide now?) https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/index.js:38: // CSS color values should be specified in hexadecimal where possible On 2017/09/13 18:12:52, Thomas Greiner wrote: > On 2017/09/11 09:26:38, ire wrote: > > I couldn't find stylelint rules for these. Not sure if I should leave the > > comments anyway? > > It's more of a guideline than a strict rule. Some more info on that (from what I > remember): > - We use `rgba()` instead of `#RRGGBBAA` for compatibility. > - Under some circumstances using `hsl()` or even `rgb()` could be more > appropriate than a hex value so prohibiting those outright is likely not a good > idea. > > Therefore `"color-named": "never"` should be sufficient for now. Okay thanks. Done. https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/index.js:42: // CSS rule declaration order should follow the WordPress CSS Coding Standards On 2017/09/13 18:12:52, Thomas Greiner wrote: > On 2017/09/11 09:26:38, ire wrote: > > The declaration order is no longer part of stylelint. If we really want this > to > > be part of our stylelint, I think we can use this plugin > > (https://www.npmjs.com/package/stylelint-order). > > Given that this rule can be quite confusing for some people it may be worth > adding unless we want to open the discussion about declaraction order again. Okay. I think we should add it in a separate commit/issue though, just so the initial config isn't held up by this (unless it is essential) https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/index.js:45: "unit-no-unknown": true, On 2017/09/13 18:12:52, Thomas Greiner wrote: > This is not what "should specify units where possible" refers to. It's about > cases like `width: 0;` where no unit is provided. Seems I misunderstood the "unit-no-unknown" rule :/ Stylelint doesn't have a rule for what we want. They have the opposite rule ("length-zero-no-unit"), which requires that zeros should not have a unit. > I don't think there's a strong reason to keep it though, in case we want to > abandon it. I personally prefer to not specify the units when its a 0, so if it's up for discussion my vote is we abandon that rule. https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/index.js:52: "indentation": 2, On 2017/09/13 18:12:52, Thomas Greiner wrote: > On 2017/09/11 09:26:38, ire wrote: > > This isn't actually documented in https://adblockplus.org/en/coding-style, but > > is a rule we seem to follow > > Under "General" we link to > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style... > which states "Two spaces per logic level, or four spaces in Python code." Acknowledged. https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... File stylelint-config-eyeo/package.json (right): https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/package.json:26: "stylelint-config-recommended": "^1.0.0" On 2017/09/13 18:12:52, Thomas Greiner wrote: > What is this dependency for? We don't have a build process that depends on it, > as far as I see. It should actually be a dependency (or peer dependency). Our config is built on top of this recommended one.
Let's discuss the coding style changes in the ticket I created. Preferrably, we should wait for those changes to be live before reflecting them in our stylelint configuration. https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... File stylelint-config-eyeo/index.js (right): https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/index.js:29: "block-closing-brace-space-before": "always-single-line", On 2017/09/14 08:33:00, ire wrote: > On 2017/09/13 18:12:52, Thomas Greiner wrote: > > Which part of the coding style does this rule refer to? > > > > Same applies to "block-opening-brace-space-after" > > We link to the Google HTML/CSS Style guide, which has rules on having a space > before opening braces - > https://google.github.io/styleguide/htmlcssguide.html#Declaration_Block_Separ... This rule is not about the opening but the closing brace. https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/index.js:31: "block-opening-brace-newline-before": "always-multi-line", On 2017/09/14 08:33:00, ire wrote: > I think I assumed that single-line rules were okay when it's just one > declaration. I can't seem to find any rule for/against this though, is this > documented somewhere? It's the "Opening braces go on their own line" guideline that I mentioned but I guess you're right because .foo {bar: baz;} would still be considered valid with that rule. I'm not sure whether stylelint considers that to be a single line though. > (If not, can we decide now?) We can always agree on changing the rules. :) Personally, I'd be happy with allowing blocks with a single selector and a single CSS property to be written in a single line. Anyway, I created https://issues.adblockplus.org/ticket/5676 to discuss what we'd like to have and to adapt the coding style accordingly. https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/index.js:42: // CSS rule declaration order should follow the WordPress CSS Coding Standards On 2017/09/14 08:33:00, ire wrote: > On 2017/09/13 18:12:52, Thomas Greiner wrote: > > On 2017/09/11 09:26:38, ire wrote: > > > The declaration order is no longer part of stylelint. If we really want this > > to > > > be part of our stylelint, I think we can use this plugin > > > (https://www.npmjs.com/package/stylelint-order). > > > > Given that this rule can be quite confusing for some people it may be worth > > adding unless we want to open the discussion about declaraction order again. > > Okay. I think we should add it in a separate commit/issue though, just so the > initial config isn't held up by this (unless it is essential) I totally agree. https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/index.js:45: "unit-no-unknown": true, On 2017/09/14 08:33:00, ire wrote: > I personally prefer to not specify the units when its a 0, so if it's up for > discussion my vote is we abandon that rule. I included this in https://issues.adblockplus.org/ticket/5676 so that we can discuss this and adapt the coding style. https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... File stylelint-config-eyeo/package.json (right): https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/package.json:26: "stylelint-config-recommended": "^1.0.0" On 2017/09/14 08:33:01, ire wrote: > On 2017/09/13 18:12:52, Thomas Greiner wrote: > > What is this dependency for? We don't have a build process that depends on it, > > as far as I see. > > It should actually be a dependency (or peer dependency). Our config is built on > top of this recommended one. Acknowledged. https://codereview.adblockplus.org/29541680/diff/29544562/stylelint-config-ey... File stylelint-config-eyeo/index.js (right): https://codereview.adblockplus.org/29541680/diff/29544562/stylelint-config-ey... stylelint-config-eyeo/index.js:31: // Use a space between the last selector and the declaration block (Google HTML/CSS Style Guide) Coding style: Line length should be 80 characters or less
> Let's discuss the coding style changes in the ticket I created. Preferrably, we > should wait for those changes to be live before reflecting them in our stylelint > configuration. Noted. I've made a few updates. https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... File stylelint-config-eyeo/index.js (right): https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/index.js:29: "block-closing-brace-space-before": "always-single-line", On 2017/09/14 10:52:15, Thomas Greiner wrote: > On 2017/09/14 08:33:00, ire wrote: > > On 2017/09/13 18:12:52, Thomas Greiner wrote: > > > Which part of the coding style does this rule refer to? > > > > > > Same applies to "block-opening-brace-space-after" > > > > We link to the Google HTML/CSS Style guide, which has rules on having a space > > before opening braces - > > > https://google.github.io/styleguide/htmlcssguide.html#Declaration_Block_Separ... > > This rule is not about the opening but the closing brace. Under 4.2.5 Declaration Block Separation, it says: "Always use a single space between the last selector and the opening brace that begins the declaration block." https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/index.js:31: "block-opening-brace-newline-before": "always-multi-line", On 2017/09/14 10:52:15, Thomas Greiner wrote: > It's the "Opening braces go on their own line" guideline that I mentioned but I > guess you're right because > > .foo > {bar: baz;} > > would still be considered valid with that rule. I'm not sure whether stylelint > considers that to be a single line though. Stylelint will consider that a single line (I believe they count the line as [opening brace, property, value, closing brace]). See this example - https://stylelint.io/user-guide/rules/block-opening-brace-newline-before/#alw... I think the example you gave is something we may not be able to catch with styelint unless we can to disallow single-line declarations completely. > We can always agree on changing the rules. :) Personally, I'd be happy with > allowing blocks with a single selector and a single CSS property to be written > in a single line. > > Anyway, I created https://issues.adblockplus.org/ticket/5676 to discuss what > we'd like to have and to adapt the coding style accordingly. Ack, thanks! https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/index.js:42: // CSS rule declaration order should follow the WordPress CSS Coding Standards On 2017/09/14 10:52:15, Thomas Greiner wrote: > On 2017/09/14 08:33:00, ire wrote: > > On 2017/09/13 18:12:52, Thomas Greiner wrote: > > > On 2017/09/11 09:26:38, ire wrote: > > > > The declaration order is no longer part of stylelint. If we really want > this > > > to > > > > be part of our stylelint, I think we can use this plugin > > > > (https://www.npmjs.com/package/stylelint-order). > > > > > > Given that this rule can be quite confusing for some people it may be worth > > > adding unless we want to open the discussion about declaraction order again. > > > > Okay. I think we should add it in a separate commit/issue though, just so the > > initial config isn't held up by this (unless it is essential) > > I totally agree. Great. Created the issue here https://issues.adblockplus.org/ticket/5689#comment:1 https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/index.js:45: "unit-no-unknown": true, On 2017/09/14 10:52:15, Thomas Greiner wrote: > On 2017/09/14 08:33:00, ire wrote: > > I personally prefer to not specify the units when its a 0, so if it's up for > > discussion my vote is we abandon that rule. > > I included this in https://issues.adblockplus.org/ticket/5676 so that we can > discuss this and adapt the coding style. Acknowledged. https://codereview.adblockplus.org/29541680/diff/29544562/stylelint-config-ey... File stylelint-config-eyeo/index.js (right): https://codereview.adblockplus.org/29541680/diff/29544562/stylelint-config-ey... stylelint-config-eyeo/index.js:31: // Use a space between the last selector and the declaration block (Google HTML/CSS Style Guide) On 2017/09/14 10:52:16, Thomas Greiner wrote: > Coding style: Line length should be 80 characters or less Done.
https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... File stylelint-config-eyeo/index.js (right): https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/index.js:29: "block-closing-brace-space-before": "always-single-line", On 2017/09/15 08:54:11, ire wrote: > Under 4.2.5 Declaration Block Separation, it says: > > "Always use a single space between the last selector and the opening brace > that begins the declaration block." There seems to be a misunderstanding - sorry about that - so I'll try to clarify it. The text you mention applies to this space: selector { name: value; } ^ The rule I'm referring to in my initial comment (i.e. "block-closing-brace-space-before") applies to this space, however: selector { name: value; } ^ The other rule in that comment (i.e. "block-opening-brace-space-after") applies to this space: selector { name: value; } ^ https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/index.js:31: "block-opening-brace-newline-before": "always-multi-line", On 2017/09/15 08:54:11, ire wrote: > I think the example you gave is something we may not be able to catch with > styelint unless we can to disallow single-line declarations completely. True, seems like it. https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/index.js:42: // CSS rule declaration order should follow the WordPress CSS Coding Standards On 2017/09/15 08:54:12, ire wrote: > Great. Created the issue here > https://issues.adblockplus.org/ticket/5689#comment:1 Awesome, thanks. https://codereview.adblockplus.org/29541680/diff/29545591/stylelint-config-ey... File stylelint-config-eyeo/README.md (right): https://codereview.adblockplus.org/29541680/diff/29545591/stylelint-config-ey... stylelint-config-eyeo/README.md:15: npm install --save-dev stylelint-config-eyeo Is this worth mentioning? Considering that there are many different ways on how you can use the package, we may end up documenting installation instructions for all possible scenarios (e.g. --save, --save-optional, grunt, gulp). Therefore I'd assume that mentioning a single use-case (in this case as a global module) and leaving the rest up to the user might be sufficient but I don't know for sure. What do you think? https://codereview.adblockplus.org/29541680/diff/29545591/stylelint-config-ey... stylelint-config-eyeo/README.md:19: npm install --g stylelint-config-eyeo Detail: This should be either `-g` or `--global`.
https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... File stylelint-config-eyeo/index.js (right): https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/index.js:29: "block-closing-brace-space-before": "always-single-line", On 2017/09/15 16:58:29, Thomas Greiner wrote: > On 2017/09/15 08:54:11, ire wrote: > > Under 4.2.5 Declaration Block Separation, it says: > > > > "Always use a single space between the last selector and the opening brace > > that begins the declaration block." > > There seems to be a misunderstanding - sorry about that - so I'll try to clarify > it. > > The text you mention applies to this space: > > selector { name: value; } > ^ > > The rule I'm referring to in my initial comment (i.e. > "block-closing-brace-space-before") applies to this space, however: > > selector { name: value; } > ^ > > The other rule in that comment (i.e. "block-opening-brace-space-after") applies > to this space: > > selector { name: value; } > ^ Ah right! Yes seems I was mixing the two up. Two questions: 1. Do you disagree with the rule as I implemented it here? i.e. is the issue that I mislabelled the rule, or that the rule is incorrect 2. Do we actually have a rule about closing braces anywhere? https://codereview.adblockplus.org/29541680/diff/29545591/stylelint-config-ey... File stylelint-config-eyeo/README.md (right): https://codereview.adblockplus.org/29541680/diff/29545591/stylelint-config-ey... stylelint-config-eyeo/README.md:15: npm install --save-dev stylelint-config-eyeo On 2017/09/15 16:58:29, Thomas Greiner wrote: > Is this worth mentioning? Considering that there are many different ways on how > you can use the package, we may end up documenting installation instructions for > all possible scenarios (e.g. --save, --save-optional, grunt, gulp). > > Therefore I'd assume that mentioning a single use-case (in this case as a global > module) and leaving the rest up to the user might be sufficient but I don't know > for sure. What do you think? I was trying to follow the style that was used in stylelint's stylelint-config-standard and stylelint-config-recommended . They mention both options (although not for the `npm install` part, just the usage part). Also, the reason I thought it better to include both, is because of the difference between how stylelint configs are extended and how eslint configs are extended. It can be a bit confusing because it seems like stylelint configs installed globally need to have an absolute path, whereas eslint configs do not. And if we mention installing only locally/globally, people may get a bit mixed up. https://codereview.adblockplus.org/29541680/diff/29545591/stylelint-config-ey... stylelint-config-eyeo/README.md:19: npm install --g stylelint-config-eyeo On 2017/09/15 16:58:29, Thomas Greiner wrote: > Detail: This should be either `-g` or `--global`. Good catch, I meant `-g`. Done. https://codereview.adblockplus.org/29541680/diff/29548555/stylelint-config-ey... File stylelint-config-eyeo/index.js (right): https://codereview.adblockplus.org/29541680/diff/29548555/stylelint-config-ey... stylelint-config-eyeo/index.js:23: // Opening braces go on their own line I think this should say "Opening and closing braces go on their own line" (with the exception of single line rules of course, if that is indeed allowed)
Here are my first impressions. I haven't read this thread yet. https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-ey... File stylelint-config-eyeo/README.md (right): https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-ey... stylelint-config-eyeo/README.md:11: This command requires administrator privileges so you might need to use `sudo`. NIT: Isn't it a bad idea to give npm administrator privileges? https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-ey... File stylelint-config-eyeo/index.js (right): https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-ey... stylelint-config-eyeo/index.js:1: /* Potential missing properties: 1. https://google.github.io/styleguide/htmlcssguide.html#Type_Selectors -> https://stylelint.io/user-guide/rules/selector-no-qualifying-type/ 2. https://google.github.io/styleguide/htmlcssguide.html#Hexadecimal_Notation -> https://stylelint.io/user-guide/rules/color-hex-length/ 3. https://google.github.io/styleguide/htmlcssguide.html#Property_Name_Stops -> https://stylelint.io/user-guide/rules/declaration-block-semicolon-space-after/ 4. https://google.github.io/styleguide/htmlcssguide.html#Property_Name_Stops -> https://stylelint.io/user-guide/rules/declaration-block-semicolon-space-before/ 5. https://google.github.io/styleguide/htmlcssguide.html#Selector_and_Declaratio... -> https://stylelint.io/user-guide/rules/selector-list-comma-newline-after/ 6. https://google.github.io/styleguide/htmlcssguide.html#Rule_Separation -> https://stylelint.io/user-guide/rules/rule-empty-line-before/ Also relevant: https://github.com/WordPress-Coding-Standards/stylelint-config-wordpress/issu... https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-ey... stylelint-config-eyeo/index.js:21: extends: "stylelint-config-recommended", NIT: We are currently using double slash comments for licence headers in SCSS files. (See [stylelint-config-recommended](https://github.com/stylelint/stylelint-config-recommended/blob/master/index.js)) https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-ey... stylelint-config-eyeo/index.js:31: // (Google HTML/CSS Style Guide) Why did you put this label here? Is everything below Google HTML/CSS until "WordPress CSS Coding Standards"? If so, shouldn't this label be above the above comment? https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-ey... stylelint-config-eyeo/index.js:43: // CSS shorthand properties usage is optional This doesn't refer to a property? (The same applies elsewhere)
Thanks Julian! I've uploaded a new patchset https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-ey... File stylelint-config-eyeo/README.md (right): https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-ey... stylelint-config-eyeo/README.md:11: This command requires administrator privileges so you might need to use `sudo`. On 2017/10/13 14:21:06, juliandoucette wrote: > NIT: Isn't it a bad idea to give npm administrator privileges? The `sudo` is what may be required to run the command. I don't think that's giving npm administrator privileges (unless I misunderstand how this works?) Also, I copied this from the eslint-config-eyeo README, so added this for consistency. https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-ey... File stylelint-config-eyeo/index.js (right): https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-ey... stylelint-config-eyeo/index.js:1: /* On 2017/10/13 14:21:06, juliandoucette wrote: > Potential missing properties: > > 1. https://google.github.io/styleguide/htmlcssguide.html#Type_Selectors -> > https://stylelint.io/user-guide/rules/selector-no-qualifying-type/ > 2. https://google.github.io/styleguide/htmlcssguide.html#Hexadecimal_Notation -> > https://stylelint.io/user-guide/rules/color-hex-length/ > 3. https://google.github.io/styleguide/htmlcssguide.html#Property_Name_Stops -> > https://stylelint.io/user-guide/rules/declaration-block-semicolon-space-after/ > 4. https://google.github.io/styleguide/htmlcssguide.html#Property_Name_Stops -> > https://stylelint.io/user-guide/rules/declaration-block-semicolon-space-before/ > 5. > https://google.github.io/styleguide/htmlcssguide.html#Selector_and_Declaratio... > -> https://stylelint.io/user-guide/rules/selector-list-comma-newline-after/ > 6. https://google.github.io/styleguide/htmlcssguide.html#Rule_Separation -> > https://stylelint.io/user-guide/rules/rule-empty-line-before/ Thanks, added > Also relevant: > https://github.com/WordPress-Coding-Standards/stylelint-config-wordpress/issu... Yup I already mentioned that. There's another issue created to add this when this has been committed. https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-ey... stylelint-config-eyeo/index.js:21: extends: "stylelint-config-recommended", On 2017/10/13 14:21:06, juliandoucette wrote: > NIT: We are currently using double slash comments for licence headers in SCSS > files. > > (See > [stylelint-config-recommended](https://github.com/stylelint/stylelint-config-recommended/blob/master/index.js)) The rule doesn't apply to SCSS files. See https://stylelint.io/user-guide/rules/no-invalid-double-slash-comments/ https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-ey... stylelint-config-eyeo/index.js:31: // (Google HTML/CSS Style Guide) On 2017/10/13 14:21:06, juliandoucette wrote: > Why did you put this label here? Is everything below Google HTML/CSS until > "WordPress CSS Coding Standards"? If so, shouldn't this label be above the above > comment? I put the label to specify that this particular rule comes from the Google HTML/CSS Style Guide, not everything below. I think I should remove those comments. What do you/everyone think? https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-ey... stylelint-config-eyeo/index.js:43: // CSS shorthand properties usage is optional On 2017/10/13 14:21:06, juliandoucette wrote: > This doesn't refer to a property? > > (The same applies elsewhere) You're right. But this is from our coding standards. So we should change it there first?
Responses. https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-ey... File stylelint-config-eyeo/README.md (right): https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-ey... stylelint-config-eyeo/README.md:11: This command requires administrator privileges so you might need to use `sudo`. On 2017/10/24 09:05:16, ire wrote: > The `sudo` is what may be required to run the command. I don't think that's > giving npm administrator privileges (unless I misunderstand how this works?) sudo is not required to run this command. > Also, I copied this from the eslint-config-eyeo README, so added this for > consistency. Ack. I will file an issue against codingtools. https://issues.adblockplus.org/ticket/5945 https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-ey... File stylelint-config-eyeo/index.js (right): https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-ey... stylelint-config-eyeo/index.js:21: extends: "stylelint-config-recommended", On 2017/10/24 09:05:16, ire wrote: > On 2017/10/13 14:21:06, juliandoucette wrote: > > NIT: We are currently using double slash comments for licence headers in SCSS > > files. > > > > (See > > > [stylelint-config-recommended](https://github.com/stylelint/stylelint-config-recommended/blob/master/index.js)) > > The rule doesn't apply to SCSS files. See > https://stylelint.io/user-guide/rules/no-invalid-double-slash-comments/ Acknowledged. https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-ey... stylelint-config-eyeo/index.js:31: // (Google HTML/CSS Style Guide) On 2017/10/24 09:05:16, ire wrote: > I think I should remove those comments. What do you/everyone think? I'm ok with: - no comments - category comments - rule comments (as long as they are not repetitive e.g. Google Style Guide above every relevant rule) https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-ey... stylelint-config-eyeo/index.js:43: // CSS shorthand properties usage is optional On 2017/10/24 09:05:16, ire wrote: > You're right. But this is from our coding standards. So we should change it > there first? I wasn't suggesting that we change our standards. I was pointing out that these comments do not apply to properties of this object. If your intention is to point out style guide rules that are missing then I would suggest that you do so more clearly e.g. by creating a clearly labeled list of rules that we haven't been able to cover yet.
New patchset! I think I've addressed most comments now. Could everyone please have another look to see if this is ready to push? https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-ey... File stylelint-config-eyeo/README.md (right): https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-ey... stylelint-config-eyeo/README.md:11: This command requires administrator privileges so you might need to use `sudo`. On 2017/10/26 15:24:08, juliandoucette wrote: > On 2017/10/24 09:05:16, ire wrote: > > The `sudo` is what may be required to run the command. I don't think that's > > giving npm administrator privileges (unless I misunderstand how this works?) > > sudo is not required to run this command. It depends on the version of npm. In older versions it was more common that people will need to use `sudo` first (I always had to do this). The comment says that you "might" need to use it. I don't think it needs to be removed, in case people are on versions where they do need to use it. > > Also, I copied this from the eslint-config-eyeo README, so added this for > > consistency. > > Ack. I will file an issue against codingtools. > > https://issues.adblockplus.org/ticket/5945 https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-ey... File stylelint-config-eyeo/index.js (right): https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-ey... stylelint-config-eyeo/index.js:31: // (Google HTML/CSS Style Guide) On 2017/10/26 15:24:08, juliandoucette wrote: > On 2017/10/24 09:05:16, ire wrote: > > I think I should remove those comments. What do you/everyone think? > > I'm ok with: > > - no comments > - category comments > - rule comments (as long as they are not repetitive e.g. Google Style Guide > above every relevant rule) Ack. I think I prefer either no comments or rule comments (I'm fine with removing the rule sources so it's less repetitive). For now, I will remove the rule sources. https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-ey... stylelint-config-eyeo/index.js:43: // CSS shorthand properties usage is optional On 2017/10/26 15:24:08, juliandoucette wrote: > On 2017/10/24 09:05:16, ire wrote: > > You're right. But this is from our coding standards. So we should change it > > there first? > > I wasn't suggesting that we change our standards. I was pointing out that these > comments do not apply to properties of this object. If your intention is to > point out style guide rules that are missing then I would suggest that you do so > more clearly e.g. by creating a clearly labeled list of rules that we haven't > been able to cover yet. Oh right! Yes I thought it was clear that if there was rule below the comment it was not covered. Since there are just the two, I'll just remove them for now.
@Thomas I'm happy to start with this. What do you think? https://codereview.adblockplus.org/29541680/diff/29592555/stylelint-config-ey... File stylelint-config-eyeo/index.js (right): https://codereview.adblockplus.org/29541680/diff/29592555/stylelint-config-ey... stylelint-config-eyeo/index.js:2: * This file is part of Adblock Plus <https://adblockplus.org/>, This isn't part of adblockplus.org https://codereview.adblockplus.org/29541680/diff/29592555/stylelint-config-ey... stylelint-config-eyeo/index.js:33: "block-opening-brace-space-after": "always-single-line", Suggest: Open before close? (very opinionated - ignore if you disagree)
https://codereview.adblockplus.org/29541680/diff/29592555/stylelint-config-ey... File stylelint-config-eyeo/index.js (right): https://codereview.adblockplus.org/29541680/diff/29592555/stylelint-config-ey... stylelint-config-eyeo/index.js:2: * This file is part of Adblock Plus <https://adblockplus.org/>, On 2017/10/31 14:22:45, juliandoucette wrote: > This isn't part of http://adblockplus.org This is the same licence header from eslint-config-eyeo https://codereview.adblockplus.org/29541680/diff/29592555/stylelint-config-ey... stylelint-config-eyeo/index.js:33: "block-opening-brace-space-after": "always-single-line", On 2017/10/31 14:22:45, juliandoucette wrote: > Suggest: Open before close? (very opinionated - ignore if you disagree) I agree. And I also think: opening before > opening after > closing before > closing after https://codereview.adblockplus.org/29541680/diff/29592555/stylelint-config-ey... stylelint-config-eyeo/index.js:33: "block-opening-brace-space-after": "always-single-line", On 2017/10/31 14:22:45, juliandoucette wrote: > Suggest: Open before close? (very opinionated - ignore if you disagree) Done.
@kzar please find question below. https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-ey... File stylelint-config-eyeo/README.md (right): https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-ey... stylelint-config-eyeo/README.md:11: This command requires administrator privileges so you might need to use `sudo`. On 2017/10/30 08:30:47, ire wrote: Let's leave this in for now and then remove it as part of https://issues.adblockplus.org/ticket/5945 if necessary. https://codereview.adblockplus.org/29541680/diff/29592555/stylelint-config-ey... File stylelint-config-eyeo/index.js (right): https://codereview.adblockplus.org/29541680/diff/29592555/stylelint-config-ey... stylelint-config-eyeo/index.js:2: * This file is part of Adblock Plus <https://adblockplus.org/>, On 2017/11/01 11:06:09, ire wrote: > On 2017/10/31 14:22:45, juliandoucette wrote: > > This isn't part of http://adblockplus.org > > This is the same licence header from eslint-config-eyeo @kzar do you consider this and eslint-config-eyeo to be part of Adblock Plus?
https://codereview.adblockplus.org/29541680/diff/29592555/stylelint-config-ey... File stylelint-config-eyeo/index.js (right): https://codereview.adblockplus.org/29541680/diff/29592555/stylelint-config-ey... stylelint-config-eyeo/index.js:2: * This file is part of Adblock Plus <https://adblockplus.org/>, On 2017/11/01 13:14:26, juliandoucette wrote: > On 2017/11/01 11:06:09, ire wrote: > > On 2017/10/31 14:22:45, juliandoucette wrote: > > > This isn't part of http://adblockplus.org > > > > This is the same licence header from eslint-config-eyeo > > @kzar do you consider this and eslint-config-eyeo to be part of Adblock Plus? I would check with legal, it's quite possible I just got it wrong. (If I used the wrong one for eslint-config-eyeo could you upload a review to fix that?)
On 2017/11/03 13:38:39, kzar wrote: > I would check with legal, it's quite possible I just got it wrong. (If I used > the wrong one for eslint-config-eyeo could you upload a review to fix that?) Yep. I'll check.
LGTM + NITs btw
On 2017/11/06 13:00:30, juliandoucette wrote: > LGTM + NITs btw What are the NITs? Or are you referring to the licence header? https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-ey... File stylelint-config-eyeo/README.md (right): https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-ey... stylelint-config-eyeo/README.md:11: This command requires administrator privileges so you might need to use `sudo`. On 2017/11/01 13:14:26, juliandoucette wrote: > On 2017/10/30 08:30:47, ire wrote: > > Let's leave this in for now and then remove it as part of > https://issues.adblockplus.org/ticket/5945 if necessary. Acknowledged.
On 2017/11/07 07:14:02, ire wrote: > On 2017/11/06 13:00:30, juliandoucette wrote: > What are the NITs? Or are you referring to the licence header? Yes. > > Let's leave this in for now and then remove it as part of > > https://issues.adblockplus.org/ticket/5945 if necessary. > > Acknowledged. Ack.
I've marked some comments as "Suggestion" because I do see the point in copying the phrasing from the style guides we refer to to keep things simple. On the other hand, we could instead link to those sections, if those comments are meant for reference, and make up our own text to reduce confusion. Therefore, I'll leave this up for discussion. https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... File stylelint-config-eyeo/index.js (right): https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/index.js:29: "block-closing-brace-space-before": "always-single-line", On 2017/09/18 07:36:20, ire wrote: > Ah right! Yes seems I was mixing the two up. > > Two questions: > 1. Do you disagree with the rule as I implemented it here? i.e. is the issue > that I mislabelled the rule, > or that the rule is incorrect > 2. Do we actually have a rule about closing braces anywhere? I can't find any mentions of it being prohibited in our coding style so it should be fine. All I could find are other uses of braces in single lines where we consistently use them without inner spaces. e.g. const {foo} = require("bar"); `foo${bar}baz` let foo = {bar: "baz"}; So I'd suggest either enforcing it without inner spaces or leaving it up to the developers to choose whichever variant they prefer. https://codereview.adblockplus.org/29541680/diff/29545591/stylelint-config-ey... File stylelint-config-eyeo/README.md (right): https://codereview.adblockplus.org/29541680/diff/29545591/stylelint-config-ey... stylelint-config-eyeo/README.md:15: npm install --save-dev stylelint-config-eyeo On 2017/09/18 07:36:20, ire wrote: > On 2017/09/15 16:58:29, Thomas Greiner wrote: > > Is this worth mentioning? Considering that there are many different ways on > how > > you can use the package, we may end up documenting installation instructions > for > > all possible scenarios (e.g. --save, --save-optional, grunt, gulp). > > > > Therefore I'd assume that mentioning a single use-case (in this case as a > global > > module) and leaving the rest up to the user might be sufficient but I don't > know > > for sure. What do you think? > > I was trying to follow the style that was used in stylelint's > stylelint-config-standard and stylelint-config-recommended . They mention both > options (although not for the `npm install` part, just the usage part). > > Also, the reason I thought it better to include both, is because of the > difference between how stylelint configs are extended and how eslint configs are > extended. It can be a bit confusing because it seems like stylelint configs > installed globally need to have an absolute path, whereas eslint configs do not. > And if we mention installing only locally/globally, people may get a bit mixed > up. Sounds reasonable. https://codereview.adblockplus.org/29541680/diff/29594569/stylelint-config-ey... File stylelint-config-eyeo/index.js (right): https://codereview.adblockplus.org/29541680/diff/29594569/stylelint-config-ey... stylelint-config-eyeo/index.js:39: // Separate selectors and declarations by new lines. Suggestion: This sentence and the one below are a bit ambiguous because they're not supposed to be separated by actual lines but by the newline character. Rephrasing it to say that each should be in their own line could resolve this. https://codereview.adblockplus.org/29541680/diff/29594569/stylelint-config-ey... stylelint-config-eyeo/index.js:45: "ignore": ["after-comment", "inside-block"] Do we actually want to specify "inside-block"? i.e. @media { a { aa: aaa; } b { bb: bbb; } } Instead, "first-nested" seems to be the option we're aiming for here. https://codereview.adblockplus.org/29541680/diff/29594569/stylelint-config-ey... stylelint-config-eyeo/index.js:54: // Use 3 character hexadecimal notation where possible. Suggestion: The rule below also encourages 4-digit hexadecimal notation over the 8-digit one. https://codereview.adblockplus.org/29541680/diff/29594569/stylelint-config-ey... stylelint-config-eyeo/index.js:69: "ignore": ["attribute"] This rule appears to refer to https://google.github.io/styleguide/htmlcssguide.html#Type_Selectors which, however, doesn't make an exception for attributes. So we should adapt our coding style to reflect that.
Thanks Thomas! On 2017/11/14 13:18:33, Thomas Greiner wrote: > I've marked some comments as "Suggestion" because I do see the point in copying > the phrasing from the style guides we refer to to keep things simple. On the > other hand, we could instead link to those sections, if those comments are meant > for reference, and make up our own text to reduce confusion. > > Therefore, I'll leave this up for discussion. Ack. https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... File stylelint-config-eyeo/index.js (right): https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/index.js:29: "block-closing-brace-space-before": "always-single-line", On 2017/11/14 13:18:30, Thomas Greiner wrote: > On 2017/09/18 07:36:20, ire wrote: > > Ah right! Yes seems I was mixing the two up. > > > > Two questions: > > 1. Do you disagree with the rule as I implemented it here? i.e. is the issue > > that I mislabelled the rule, > > or that the rule is incorrect > > 2. Do we actually have a rule about closing braces anywhere? > > I can't find any mentions of it being prohibited in our coding style so it > should be fine. All I could find are other uses of braces in single lines where > we consistently use them without inner spaces. > > e.g. > const {foo} = require("bar"); > `foo${bar}baz` > let foo = {bar: "baz"}; > > So I'd suggest either enforcing it without inner spaces or leaving it up to the > developers to choose whichever variant they prefer. Okay. I will remove two rules: "block-opening-brace-space-after" and "block-closing-brace-space-before" https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-ey... stylelint-config-eyeo/index.js:29: "block-closing-brace-space-before": "always-single-line", On 2017/11/14 13:18:30, Thomas Greiner wrote: > On 2017/09/18 07:36:20, ire wrote: > > Ah right! Yes seems I was mixing the two up. > > > > Two questions: > > 1. Do you disagree with the rule as I implemented it here? i.e. is the issue > > that I mislabelled the rule, > > or that the rule is incorrect > > 2. Do we actually have a rule about closing braces anywhere? > > I can't find any mentions of it being prohibited in our coding style so it > should be fine. All I could find are other uses of braces in single lines where > we consistently use them without inner spaces. > > e.g. > const {foo} = require("bar"); > `foo${bar}baz` > let foo = {bar: "baz"}; > > So I'd suggest either enforcing it without inner spaces or leaving it up to the > developers to choose whichever variant they prefer. Done. https://codereview.adblockplus.org/29541680/diff/29594569/stylelint-config-ey... File stylelint-config-eyeo/index.js (right): https://codereview.adblockplus.org/29541680/diff/29594569/stylelint-config-ey... stylelint-config-eyeo/index.js:39: // Separate selectors and declarations by new lines. On 2017/11/14 13:18:32, Thomas Greiner wrote: > Suggestion: This sentence and the one below are a bit ambiguous because they're > not supposed to be separated by actual lines but by the newline character. > > Rephrasing it to say that each should be in their own line could resolve this. I agree with you. Done. https://codereview.adblockplus.org/29541680/diff/29594569/stylelint-config-ey... stylelint-config-eyeo/index.js:45: "ignore": ["after-comment", "inside-block"] On 2017/11/14 13:18:32, Thomas Greiner wrote: > Do we actually want to specify "inside-block"? > > i.e. > @media > { > a > { > aa: aaa; > } > b > { > bb: bbb; > } > } > > Instead, "first-nested" seems to be the option we're aiming for here. You're right. Done. https://codereview.adblockplus.org/29541680/diff/29594569/stylelint-config-ey... stylelint-config-eyeo/index.js:54: // Use 3 character hexadecimal notation where possible. On 2017/11/14 13:18:31, Thomas Greiner wrote: > Suggestion: The rule below also encourages 4-digit hexadecimal notation over the > 8-digit one. Sorry I don't understand. Is your suggestion that I add that phrase to the comment describing this rule? https://codereview.adblockplus.org/29541680/diff/29594569/stylelint-config-ey... stylelint-config-eyeo/index.js:69: "ignore": ["attribute"] On 2017/11/14 13:18:32, Thomas Greiner wrote: > This rule appears to refer to > https://google.github.io/styleguide/htmlcssguide.html#Type_Selectors which, > however, doesn't make an exception for attributes. So we should adapt our coding > style to reflect that. The way I read that rule was that it implicitly allowed for attributes, since they specifically mentioned only IDs and class names.
The one remaining comment is optional since it's merely a suggestion. LGTM https://codereview.adblockplus.org/29541680/diff/29594569/stylelint-config-ey... File stylelint-config-eyeo/index.js (right): https://codereview.adblockplus.org/29541680/diff/29594569/stylelint-config-ey... stylelint-config-eyeo/index.js:54: // Use 3 character hexadecimal notation where possible. On 2017/11/15 06:38:32, ire wrote: > On 2017/11/14 13:18:31, Thomas Greiner wrote: > > Suggestion: The rule below also encourages 4-digit hexadecimal notation over > the > > 8-digit one. > > Sorry I don't understand. Is your suggestion that I add that phrase to the > comment describing this rule? I just wanted to point out that this statement doesn't match what the rule does. But if you want a suggestion on how to resolve this, we could either rephrase "3 character hexadecimal notation" as "short hexadecimal notation" or leave it as is and not ignore the fact that the rule does more than what's required of it. https://codereview.adblockplus.org/29541680/diff/29594569/stylelint-config-ey... stylelint-config-eyeo/index.js:69: "ignore": ["attribute"] On 2017/11/15 06:38:33, ire wrote: > On 2017/11/14 13:18:32, Thomas Greiner wrote: > > This rule appears to refer to > > https://google.github.io/styleguide/htmlcssguide.html#Type_Selectors which, > > however, doesn't make an exception for attributes. So we should adapt our > coding > > style to reflect that. > > The way I read that rule was that it implicitly allowed for attributes, since > they specifically mentioned only IDs and class names. Very good point. I guess I assumed that they implied that it also applied to other selector types (e.g. `[foo]`, `:foo`, `::foo`) but reading it again, I agree with your assessment.
Thanks Thomas. I took your suggestion, will commit this now https://codereview.adblockplus.org/29541680/diff/29594569/stylelint-config-ey... File stylelint-config-eyeo/index.js (right): https://codereview.adblockplus.org/29541680/diff/29594569/stylelint-config-ey... stylelint-config-eyeo/index.js:54: // Use 3 character hexadecimal notation where possible. On 2017/11/15 12:22:00, Thomas Greiner wrote: > On 2017/11/15 06:38:32, ire wrote: > > On 2017/11/14 13:18:31, Thomas Greiner wrote: > > > Suggestion: The rule below also encourages 4-digit hexadecimal notation over > > the > > > 8-digit one. > > > > Sorry I don't understand. Is your suggestion that I add that phrase to the > > comment describing this rule? > > I just wanted to point out that this statement doesn't match what the rule does. > > But if you want a suggestion on how to resolve this, we could either rephrase "3 > character hexadecimal notation" as "short hexadecimal notation" or leave it as > is and not ignore the fact that the rule does more than what's required of it. Ah I see what you mean (that completely flew over my head :*) I like your suggestion to use "short hexadecimal notation" instead. Done.
It was brought to my attention that this hasn't been reviewed by the module owner or peer, so re-opening this review and adding Sebastian (Module Owner) and Tristan (Module Peer).
On 2017/11/30 15:25:17, ire wrote: > It was brought to my attention that this hasn't been reviewed by the module > owner or peer, so re-opening this review and adding Sebastian (Module Owner) and > Tristan (Module Peer). hasn't been reviewed by the codingtools* module owner or peer
On 2017/11/30 15:26:29, ire wrote: > On 2017/11/30 15:25:17, ire wrote: > > It was brought to my attention that this hasn't been reviewed by the module > > owner or peer, so re-opening this review and adding Sebastian (Module Owner) > and > > Tristan (Module Peer). > > hasn't been reviewed by the codingtools* module owner or peer Applying the diff raises 4 whitespace errors: git apply issue29541680_29609595.diff issue29541680_29609595.diff:9: trailing whitespace. A [Stylelint](https://stylelint.io) configuration that checks for compliance issue29541680_29609595.diff:10: trailing whitespace. with the [Adblock Plus coding style guide](https://adblockplus.org/coding-style#html-css) issue29541680_29609595.diff:29: trailing whitespace. To lint a CSS (or CSS-like, e.g. SCSS, SugarCSS, Less) file using Stylelint issue29541680_29609595.diff:43: trailing whitespace. If you've globally installed stylelint-config-eyeo using the `-g` flag, then warning: 4 lines add whitespace errors. (All of them seem to be located in README.md) Would you mind resolving them? The rest looks good to me though. Best, Tristan
On 2017/12/14 10:20:18, tlucas wrote: > On 2017/11/30 15:26:29, ire wrote: > > On 2017/11/30 15:25:17, ire wrote: > > > It was brought to my attention that this hasn't been reviewed by the module > > > owner or peer, so re-opening this review and adding Sebastian (Module Owner) > > and > > > Tristan (Module Peer). > > > > hasn't been reviewed by the codingtools* module owner or peer > > Applying the diff raises 4 whitespace errors: > > git apply issue29541680_29609595.diff > issue29541680_29609595.diff:9: trailing whitespace. > A [Stylelint](https://stylelint.io) configuration that checks for compliance > issue29541680_29609595.diff:10: trailing whitespace. > with the [Adblock Plus coding style > guide](https://adblockplus.org/coding-style#html-css) > issue29541680_29609595.diff:29: trailing whitespace. > To lint a CSS (or CSS-like, e.g. SCSS, SugarCSS, Less) file using Stylelint > issue29541680_29609595.diff:43: trailing whitespace. > If you've globally installed stylelint-config-eyeo using the `-g` flag, then > warning: 4 lines add whitespace errors. > > (All of them seem to be located in README.md) > Would you mind resolving them? The rest looks good to me though. > > Best, > Tristan Thanks Tristan! Done.
On 2017/12/14 10:38:58, ire wrote: > On 2017/12/14 10:20:18, tlucas wrote: > > On 2017/11/30 15:26:29, ire wrote: > > > On 2017/11/30 15:25:17, ire wrote: > > > > It was brought to my attention that this hasn't been reviewed by the > module > > > > owner or peer, so re-opening this review and adding Sebastian (Module > Owner) > > > and > > > > Tristan (Module Peer). > > > > > > hasn't been reviewed by the codingtools* module owner or peer > > > > Applying the diff raises 4 whitespace errors: > > > > git apply issue29541680_29609595.diff > > issue29541680_29609595.diff:9: trailing whitespace. > > A [Stylelint](https://stylelint.io) configuration that checks for compliance > > issue29541680_29609595.diff:10: trailing whitespace. > > with the [Adblock Plus coding style > > guide](https://adblockplus.org/coding-style#html-css) > > issue29541680_29609595.diff:29: trailing whitespace. > > To lint a CSS (or CSS-like, e.g. SCSS, SugarCSS, Less) file using Stylelint > > issue29541680_29609595.diff:43: trailing whitespace. > > If you've globally installed stylelint-config-eyeo using the `-g` flag, then > > warning: 4 lines add whitespace errors. > > > > (All of them seem to be located in README.md) > > Would you mind resolving them? The rest looks good to me though. > > > > Best, > > Tristan > > Thanks Tristan! Done. LGTM ;)
On 2017/12/14 10:40:02, tlucas wrote: > On 2017/12/14 10:38:58, ire wrote: > > On 2017/12/14 10:20:18, tlucas wrote: > > > On 2017/11/30 15:26:29, ire wrote: > > > > On 2017/11/30 15:25:17, ire wrote: > > > > > It was brought to my attention that this hasn't been reviewed by the > > module > > > > > owner or peer, so re-opening this review and adding Sebastian (Module > > Owner) > > > > and > > > > > Tristan (Module Peer). > > > > > > > > hasn't been reviewed by the codingtools* module owner or peer > > > > > > Applying the diff raises 4 whitespace errors: > > > > > > git apply issue29541680_29609595.diff > > > issue29541680_29609595.diff:9: trailing whitespace. > > > A [Stylelint](https://stylelint.io) configuration that checks for > compliance > > > issue29541680_29609595.diff:10: trailing whitespace. > > > with the [Adblock Plus coding style > > > guide](https://adblockplus.org/coding-style#html-css) > > > issue29541680_29609595.diff:29: trailing whitespace. > > > To lint a CSS (or CSS-like, e.g. SCSS, SugarCSS, Less) file using Stylelint > > > > issue29541680_29609595.diff:43: trailing whitespace. > > > If you've globally installed stylelint-config-eyeo using the `-g` flag, > then > > > warning: 4 lines add whitespace errors. > > > > > > (All of them seem to be located in README.md) > > > Would you mind resolving them? The rest looks good to me though. > > > > > > Best, > > > Tristan > > > > Thanks Tristan! Done. > > LGTM ;) Great :) Will you or Sebastian be able to help commit/push this as I don't have access to do so myself? 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
