Issue 29360001: Issue #5109 - Create stylelintrc for websites and ui modules (Closed)
|
Created:
Oct. 27, 2016, 12:28 p.m. by juliandoucette Modified:
July 24, 2017, 4:18 p.m. Visibility:
Public. |
DescriptionCOLLABORATOR=manvel@adblockplus.org
COLLABORATOR=thomas@adblockplus.org
COLLABORATOR=i.aderinokun@eyeo.com
Patch Set 1 #
Total comments: 2
Patch Set 2 : See comments inline #
Total comments: 10
Patch Set 3 : Changes outlined in comments #
Total comments: 1
Patch Set 4 : Updated browser support based on ABP requirements #
Total comments: 11
MessagesTotal messages: 9
- Drafted stylelintrc - Reviewed stylelintrc - Tested stylelintrc against web.adblockplus.org and web.acceptableads.com
- Updated description
Several fixes below. https://codereview.adblockplus.org/29360001/diff/29360002/stylelintrc.json File stylelintrc.json (right): https://codereview.adblockplus.org/29360001/diff/29360002/stylelintrc.json#ne... stylelintrc.json:32: "unit-whitelist": ["px", "%"], Removed because whitelist requires array per property. https://codereview.adblockplus.org/29360001/diff/29360002/stylelintrc.json#ne... stylelintrc.json:56: "declaration-property-unit-whitelist": ["px", "%"], Removed because whitelist requires array per property. https://codereview.adblockplus.org/29360001/diff/29362148/stylelintrc.json File stylelintrc.json (right): https://codereview.adblockplus.org/29360001/diff/29362148/stylelintrc.json#ne... stylelintrc.json:6: "color-no-invalid-hex": true, Fixed invalid property value. https://codereview.adblockplus.org/29360001/diff/29362148/stylelintrc.json#ne... stylelintrc.json:207: "declaration-block-semicolon-newline-before": "never-multi-line", Fixed invalid property value. https://codereview.adblockplus.org/29360001/diff/29362148/stylelintrc.json#ne... stylelintrc.json:220: "block-opening-brace-newline-after": "always", Corrected. https://codereview.adblockplus.org/29360001/diff/29362148/stylelintrc.json#ne... stylelintrc.json:234: "selector-no-qualifying-type": [true, {"ignore": "attribute"}], - Fixed invalid property value - Added ignore attribute (EG: [dir], [type]) https://codereview.adblockplus.org/29360001/diff/29362148/stylelintrc.json#ne... stylelintrc.json:247: "selector-list-comma-newline-before": "never-multi-line", Fixed invalid property value. https://codereview.adblockplus.org/29360001/diff/29362148/stylelintrc.json#ne... stylelintrc.json:265: "media-query-list-comma-newline-after": "never-multi-line", Fixed invalid property value. https://codereview.adblockplus.org/29360001/diff/29362148/stylelintrc.json#ne... stylelintrc.json:266: "media-query-list-comma-newline-before": "never-multi-line", Fixed invalid property value. https://codereview.adblockplus.org/29360001/diff/29362148/stylelintrc.json#ne... stylelintrc.json:287: "no-unsupported-browser-features": [true, {"browsers": "last 2 versions, > 1%, IE 8"}] - Fixed invalid property value - Changed rule to be more inclusive
Another update. Feel free to use and contribute. https://codereview.adblockplus.org/29360001/diff/29362148/stylelintrc.json File stylelintrc.json (right): https://codereview.adblockplus.org/29360001/diff/29362148/stylelintrc.json#ne... stylelintrc.json:271: "at-rule-name-space-after": "always", Removed because we don't want to force this. https://codereview.adblockplus.org/29360001/diff/29362148/stylelintrc.json#ne... stylelintrc.json:285: "no-indistinguishable-colors": true, Removed because it creates false positives with SCSS variables. https://codereview.adblockplus.org/29360001/diff/29362166/stylelintrc.json File stylelintrc.json (right): https://codereview.adblockplus.org/29360001/diff/29362166/stylelintrc.json#ne... stylelintrc.json:289: "ignore": ["css-gencontent", "css-mediaqueries"] Added to ignore because we polyfill these features in IE8.
I'll try using this in the help center to get some real world test of it. https://codereview.adblockplus.org/29360001/diff/29362207/stylelintrc.json File stylelintrc.json (right): https://codereview.adblockplus.org/29360001/diff/29362207/stylelintrc.json#ne... stylelintrc.json:288: "browsers": "last 2 versions, Safari 6, IE 8", Shouldn't this be "last 2 versions, IE >= 9" ? https://codereview.adblockplus.org/29360001/diff/29362207/stylelintrc.json#ne... stylelintrc.json:289: "ignore": ["css-gencontent", "css-mediaqueries"] What about feature queries?
https://codereview.adblockplus.org/29360001/diff/29362207/stylelintrc.json File stylelintrc.json (right): https://codereview.adblockplus.org/29360001/diff/29362207/stylelintrc.json#ne... stylelintrc.json:288: "browsers": "last 2 versions, Safari 6, IE 8", On 2017/07/07 15:51:29, ire wrote: > Shouldn't this be "last 2 versions, IE >= 9" ? Perhaps. We haven't set this is stone yet. https://codereview.adblockplus.org/29360001/diff/29362207/stylelintrc.json#ne... stylelintrc.json:289: "ignore": ["css-gencontent", "css-mediaqueries"] On 2017/07/07 15:51:28, ire wrote: > What about feature queries? I didn't know that they existed at the time I made this :D
Hope you don't mind me being a bit more thorough with this one but I feel it's a good basis for future improvements to our coding style. Generally, be careful when enforcing more rules than what's in the coding style. Felix can give you better feedback on that if you're interested in why we want to dictate as little as necessary. https://codereview.adblockplus.org/29360001/diff/29362207/stylelintrc.json File stylelintrc.json (right): https://codereview.adblockplus.org/29360001/diff/29362207/stylelintrc.json#ne... stylelintrc.json:1: { Did you also consider the "General" section of our coding style? The reason why I'm wondering is because rules such as "max-line-length" are not present. https://codereview.adblockplus.org/29360001/diff/29362207/stylelintrc.json#ne... stylelintrc.json:12: "function-comma-newline-before": "never-multi-line", Where's this mentioned in the coding style? In general, there seem to be a few rules here that are not based on our coding style so I won't comment on each of those. We should be very careful with enforcing additional rules to avoid unnecessary conflicts. That being said, I'd assume that rules that check for invalid syntax and errors (e.g. "color-no-invalid-hex" or "string-no-newline") should still be fine. https://codereview.adblockplus.org/29360001/diff/29362207/stylelintrc.json#ne... stylelintrc.json:19: "function-url-quotes": "always", This is the opposite of what our coding style defines (see https://google.github.io/styleguide/htmlcssguide.html#CSS_Quotation_Marks) and most of our code doesn't use quotation marks for URLs. https://codereview.adblockplus.org/29360001/diff/29362207/stylelintrc.json#ne... stylelintrc.json:28: "time-no-imperceptible": true, Apart from it not being in the coding style, this rule also appears to be deprecated (see https://github.com/stylelint/stylelint/tree/master/lib/rules/time-no-impercep...). I also noticed other rules which have been deprecated (e.g. "custom-property-no-outside-root"). https://codereview.adblockplus.org/29360001/diff/29362207/stylelintrc.json#ne... stylelintrc.json:54: "declaration-no-important": true, While this is not part of the coding style, I'd argue that it still makes sense. Of course, there are situations in which it is perfectly fine to use `!important` but forcing someone to explicitly disable this rule for such a line is probably a good idea. It might make sense to mention that in our coding style, however, that it is highly discouraged to use `!important`. https://codereview.adblockplus.org/29360001/diff/29362207/stylelintrc.json#ne... stylelintrc.json:60: "declaration-block-properties-order": [ We have also discussed the approach of enumerating properties for CSS linting of our Flattr extension and decided against it. Unfortunately, I couldn't find the thread anymore to look up our arguments from back then. At least its maintainability and stiffness are significant arguments against it. Therefore solutions that allow us to define the order of property groups rather than individual properties would be more appropriate since that's all the coding style dictates. Fortunately, it seems that stylelint supports that so here's an example: e.g. [ { "order": "flexible", "properties": [ "display", "visibility" ] }, { "order": "flexible", "properties": [ "bottom", "clear", "float", "left", "position", "right", "top", "z-index" ] }, ... ] However, according to https://github.com/stylelint/stylelint/tree/master/lib/rules/declaration-bloc... that rule has been deprecated. https://codereview.adblockplus.org/29360001/diff/29362207/stylelintrc.json#ne... stylelintrc.json:285: "no-unsupported-browser-features": [ Note that the requirements for the extension UIs depend on the platforms they're being shipped in. For instance, our upcoming mobile options page won't need to be compatible with other browsers than Firefox Mobile. Therefore this seems to be one of the few rules that we shouldn't define globally.
Closing - see https://issues.adblockplus.org/ticket/5109#comment:5 I'll update/redo this soon (I took Erik's suggestion to bump it in priority) if nobody else volunteers. 
|
