Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(309)

Issue 29541680: Issue 5109 - Create stylelintrc for websites and ui modules (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 2 months ago by ire
Modified:
1 year, 11 months ago
CC:
kzar
Base URL:
https://hg.adblockplus.org/codingtools
Visibility:
Public.

Description

Issue 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+146 lines, -0 lines) Patch
A stylelint-config-eyeo/README.md View 1 2 3 4 5 6 7 8 9 10 1 chunk +45 lines, -0 lines 0 comments Download
A stylelint-config-eyeo/index.js View 1 2 3 4 5 6 7 8 9 1 chunk +70 lines, -0 lines 0 comments Download
A stylelint-config-eyeo/package.json View 1 1 chunk +31 lines, -0 lines 0 comments Download

Messages

Total messages: 30
ire
2 years, 2 months ago (2017-09-11 09:21:12 UTC) #1
ire
Ready for review. https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-eyeo/index.js File stylelint-config-eyeo/index.js (right): https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-eyeo/index.js#newcode38 stylelint-config-eyeo/index.js:38: // CSS color values should be ...
2 years, 2 months ago (2017-09-11 09:26:38 UTC) #2
Thomas Greiner
https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-eyeo/README.md File stylelint-config-eyeo/README.md (right): https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-eyeo/README.md#newcode3 stylelint-config-eyeo/README.md:3: A [Styelint](https://stylelint.io) configuration that checks for compliance with the ...
2 years, 2 months ago (2017-09-13 18:12:53 UTC) #3
ire
Thanks Thomas! I had just copied most of the README/package.json from the eslint-config project and ...
2 years, 2 months ago (2017-09-14 08:33:01 UTC) #4
Thomas Greiner
Let's discuss the coding style changes in the ticket I created. Preferrably, we should wait ...
2 years, 2 months ago (2017-09-14 10:52:16 UTC) #5
ire
> Let's discuss the coding style changes in the ticket I created. Preferrably, we > ...
2 years, 2 months ago (2017-09-15 08:54:12 UTC) #6
Thomas Greiner
https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-eyeo/index.js File stylelint-config-eyeo/index.js (right): https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-eyeo/index.js#newcode29 stylelint-config-eyeo/index.js:29: "block-closing-brace-space-before": "always-single-line", On 2017/09/15 08:54:11, ire wrote: > Under ...
2 years, 2 months ago (2017-09-15 16:58:30 UTC) #7
ire
https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-eyeo/index.js File stylelint-config-eyeo/index.js (right): https://codereview.adblockplus.org/29541680/diff/29541681/stylelint-config-eyeo/index.js#newcode29 stylelint-config-eyeo/index.js:29: "block-closing-brace-space-before": "always-single-line", On 2017/09/15 16:58:29, Thomas Greiner wrote: > ...
2 years, 2 months ago (2017-09-18 07:36:21 UTC) #8
juliandoucette
Here are my first impressions. I haven't read this thread yet. https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-eyeo/README.md File stylelint-config-eyeo/README.md (right): ...
2 years, 1 month ago (2017-10-13 14:21:07 UTC) #9
ire
Thanks Julian! I've uploaded a new patchset https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-eyeo/README.md File stylelint-config-eyeo/README.md (right): https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-eyeo/README.md#newcode11 stylelint-config-eyeo/README.md:11: This command ...
2 years, 1 month ago (2017-10-24 09:05:17 UTC) #10
juliandoucette
Responses. https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-eyeo/README.md File stylelint-config-eyeo/README.md (right): https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-eyeo/README.md#newcode11 stylelint-config-eyeo/README.md:11: This command requires administrator privileges so you might ...
2 years, 1 month ago (2017-10-26 15:24:09 UTC) #11
ire
New patchset! I think I've addressed most comments now. Could everyone please have another look ...
2 years, 1 month ago (2017-10-30 08:30:48 UTC) #12
juliandoucette
@Thomas I'm happy to start with this. What do you think? https://codereview.adblockplus.org/29541680/diff/29592555/stylelint-config-eyeo/index.js File stylelint-config-eyeo/index.js (right): ...
2 years, 1 month ago (2017-10-31 14:22:45 UTC) #13
ire
https://codereview.adblockplus.org/29541680/diff/29592555/stylelint-config-eyeo/index.js File stylelint-config-eyeo/index.js (right): https://codereview.adblockplus.org/29541680/diff/29592555/stylelint-config-eyeo/index.js#newcode2 stylelint-config-eyeo/index.js:2: * This file is part of Adblock Plus <https://adblockplus.org/>, ...
2 years, 1 month ago (2017-11-01 11:06:10 UTC) #14
juliandoucette
@kzar please find question below. https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-eyeo/README.md File stylelint-config-eyeo/README.md (right): https://codereview.adblockplus.org/29541680/diff/29548563/stylelint-config-eyeo/README.md#newcode11 stylelint-config-eyeo/README.md:11: This command requires administrator ...
2 years, 1 month ago (2017-11-01 13:14:27 UTC) #15
kzar
https://codereview.adblockplus.org/29541680/diff/29592555/stylelint-config-eyeo/index.js File stylelint-config-eyeo/index.js (right): https://codereview.adblockplus.org/29541680/diff/29592555/stylelint-config-eyeo/index.js#newcode2 stylelint-config-eyeo/index.js:2: * This file is part of Adblock Plus <https://adblockplus.org/>, ...
2 years, 1 month ago (2017-11-03 13:38:39 UTC) #16
juliandoucette
On 2017/11/03 13:38:39, kzar wrote: > I would check with legal, it's quite possible I ...
2 years ago (2017-11-06 12:38:29 UTC) #17
juliandoucette
LGTM + NITs btw
2 years ago (2017-11-06 13:00:30 UTC) #18
ire
On 2017/11/06 13:00:30, juliandoucette wrote: > LGTM + NITs btw What are the NITs? Or ...
2 years ago (2017-11-07 07:14:02 UTC) #19
juliandoucette
On 2017/11/07 07:14:02, ire wrote: > On 2017/11/06 13:00:30, juliandoucette wrote: > What are the ...
2 years ago (2017-11-07 12:49:24 UTC) #20
Thomas Greiner
I've marked some comments as "Suggestion" because I do see the point in copying the ...
2 years ago (2017-11-14 13:18:33 UTC) #21
ire
Thanks Thomas! On 2017/11/14 13:18:33, Thomas Greiner wrote: > I've marked some comments as "Suggestion" ...
2 years ago (2017-11-15 06:38:34 UTC) #22
Thomas Greiner
The one remaining comment is optional since it's merely a suggestion. LGTM https://codereview.adblockplus.org/29541680/diff/29594569/stylelint-config-eyeo/index.js File stylelint-config-eyeo/index.js ...
2 years ago (2017-11-15 12:22:00 UTC) #23
ire
Thanks Thomas. I took your suggestion, will commit this now https://codereview.adblockplus.org/29541680/diff/29594569/stylelint-config-eyeo/index.js File stylelint-config-eyeo/index.js (right): https://codereview.adblockplus.org/29541680/diff/29594569/stylelint-config-eyeo/index.js#newcode54 ...
2 years ago (2017-11-15 17:04:02 UTC) #24
ire
It was brought to my attention that this hasn't been reviewed by the module owner ...
2 years ago (2017-11-30 15:25:17 UTC) #25
ire
On 2017/11/30 15:25:17, ire wrote: > It was brought to my attention that this hasn't ...
2 years ago (2017-11-30 15:26:29 UTC) #26
tlucas
On 2017/11/30 15:26:29, ire wrote: > On 2017/11/30 15:25:17, ire wrote: > > It was ...
1 year, 11 months ago (2017-12-14 10:20:18 UTC) #27
ire
On 2017/12/14 10:20:18, tlucas wrote: > On 2017/11/30 15:26:29, ire wrote: > > On 2017/11/30 ...
1 year, 11 months ago (2017-12-14 10:38:58 UTC) #28
tlucas
On 2017/12/14 10:38:58, ire wrote: > On 2017/12/14 10:20:18, tlucas wrote: > > On 2017/11/30 ...
1 year, 11 months ago (2017-12-14 10:40:02 UTC) #29
ire
1 year, 11 months ago (2017-12-14 10:41:29 UTC) #30
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?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5