Issue 29643562: Issue 5689 - Add rules for CSS declaration order to stylelint-config-eyeo (Closed)
|
Created:
Dec. 18, 2017, 9:14 a.m. by ire Modified:
Feb. 8, 2018, 3:59 p.m. Base URL:
https://hg.adblockplus.org/codingtools Visibility:
Public. |
DescriptionIssue 5689 - Add rules for CSS declaration order to stylelint-config-eyeo
Patch Set 1 #
Total comments: 3
Patch Set 2 : Rebase, ignore node_modules in any subdirectory #
Total comments: 2
Patch Set 3 : Fkexible order of properties #
Total comments: 6
Patch Set 4 : Fixed trailing whitespace errors #
MessagesTotal messages: 19
https://codereview.adblockplus.org/29643562/diff/29643563/stylelint-config-ey... File stylelint-config-eyeo/css-properties-order.js (right): https://codereview.adblockplus.org/29643562/diff/29643563/stylelint-config-ey... stylelint-config-eyeo/css-properties-order.js:20: module.exports = [ This was essentially copied from this file -> https://github.com/cedaro/grunt-wp-css/blob/develop/tasks/config/default.json The only thing I changed was to remove the vendor-prefixed versions. Although this is ordered according to the general wordpress guide [Display, Positioning, Box model, Colors and Typography, Other], its not clear how they are ordering it within these blocks. Seems a bit random to me. So, we can either keep it this way, or order alphabetically within these blocks. Any thoughts?
Relevant background information: https://codereview.adblockplus.org/29360001/diff/29362207/stylelintrc.json#ne...
On 2017/12/18 15:50:11, juliandoucette wrote: > Relevant background information: > https://codereview.adblockplus.org/29360001/diff/29362207/stylelintrc.json#ne... AFAIK there is no way to specify the order using broad categories like we would want. I think writing out the full properties is the only way to get the desired order
I'm torn because I think that linting individual property order may be too strict. Do you think it's a productive use of our time to fix not-categorical (or categorical, for that matter) property order issues?
On 2017/12/18 17:21:54, juliandoucette wrote: > I'm torn because I think that linting individual property order may be too > strict. I don't mind linting individual property order. It's strict, but I don't think (while writing new code) it adds too much overhead and I like that there is an order to things > Do you think it's a productive use of our time to fix not-categorical > (or categorical, for that matter) property order issues? I don't think we need to go back and fix property order issues that may already exist, but if we happen to work on some old code, or for our new code, we can gradually correct the property order that way.
What do you think Thomas?
On 2018/01/04 15:26:35, juliandoucette wrote: > What do you think Thomas? 1) "declaration-block-properties-order" seems to be deprecated in favor of the "stylelint-order" plugin. (see https://github.com/stylelint/stylelint/issues/2010) 2) As I mentioned in the review comment Ire's referring to previously, I don't think we should micromanage things such as forcing "bottom" to always appear before "right" but rather to define properties that should appear before other properties. See also `order: flexible` at https://github.com/hudochenkov/stylelint-order/blob/master/rules/properties-o... and the example I gave in my review comment. 3) We shouldn't introduce anything that is incompatible with the majority of our existing code. However, if there's no consistency in the existing code, we could follow the suggested approach of fixing it over time so that we don't have to change everything at once.
Thanks Thomas! On 2018/01/08 14:40:34, Thomas Greiner wrote: > 2) As I mentioned in the review comment Ire's referring to previously, I don't > think we should micromanage things such as forcing "bottom" to always appear > before "right" but rather to define properties that should appear before other > properties. See also `order: flexible` at > https://github.com/hudochenkov/stylelint-order/blob/master/rules/properties-o... > and the example I gave in my review comment. The problem with this is the Wordpress Coding Standards specifically mention using the "Top/Right/Bottom/Left (TRBL/trouble)" order. So unless we want to override this, we may have to stick with this micromanaging method. > 3) We shouldn't introduce anything that is incompatible with the majority of our > existing code. I agree with this in theory, but it seems that the issue we actually have is that we haven't had a way of enforcing this rule, which has been part of our coding guidelines. Unless we want to change this rule to be compatible with our existing code, I don't think there is any way around having part of our existing code "incorrect" while we adjust. > However, if there's no consistency in the existing code, we could > follow the suggested approach of fixing it over time so that we don't have to > change everything at once. I agree :)
Please ignore the first point in my previous comment. I don't know why I mentioned that since "stylelint-order" is already used anyway so that point is redundant. On 2018/01/09 08:16:30, ire wrote: > The problem with this is the Wordpress Coding Standards specifically mention > using the "Top/Right/Bottom/Left (TRBL/trouble)" order. So unless we want to > override this, we may have to stick with this micromanaging method. You're right, it was a bad example. :) My point was not specific to TRBL properties but more general so I could've picked any two properties within a group. For instance, we shouldn't enforce that "background-color" must be placed before "background-image" because there's no basis for that in the coding style. Fortunately, we don't have to enforce everything in our coding style. The important thing is that we can tackle the biggest pain points while not restricting developers more than what the coding style requires to avoid frustration. If TRBL ordering turns out to be too annoying we can always revisit it later on. Sounds good? > I agree with this in theory, but it seems that the issue we actually have is > that we haven't had a way of enforcing this rule, which has been part of our > coding guidelines. Unless we want to change this rule to be compatible with our > existing code, I don't think there is any way around having part of our existing > code "incorrect" while we adjust. I see it similarly. We'll probably have to live with it for a while initially. https://codereview.adblockplus.org/29643562/diff/29643563/.hgignore File .hgignore (right): https://codereview.adblockplus.org/29643562/diff/29643563/.hgignore#newcode11 .hgignore:11: stylelint-config-eyeo/node_modules/ Detail: What about making this more generic and writing only "node_modules/" so that it can be used in any directory in the repository?
On 2018/01/09 14:15:05, Thomas Greiner wrote: > Please ignore the first point in my previous comment. I don't know why I > mentioned that since "stylelint-order" is already used anyway so that point is > redundant. Ack. > You're right, it was a bad example. :) My point was not specific to TRBL > properties but more general so I could've picked any two properties within a > group. For instance, we shouldn't enforce that "background-color" must be placed > before "background-image" because there's no basis for that in the coding style. > > Fortunately, we don't have to enforce everything in our coding style. The > important thing is that we can tackle the biggest pain points while not > restricting developers more than what the coding style requires to avoid > frustration. > > If TRBL ordering turns out to be too annoying we can always revisit it later on. > Sounds good? Sounds good :) https://codereview.adblockplus.org/29643562/diff/29643563/.hgignore File .hgignore (right): https://codereview.adblockplus.org/29643562/diff/29643563/.hgignore#newcode11 .hgignore:11: stylelint-config-eyeo/node_modules/ On 2018/01/09 14:15:05, Thomas Greiner wrote: > Detail: What about making this more generic and writing only "node_modules/" so > that it can be used in any directory in the repository? Great idea, done.
I'm ok with trying this. LGTM. https://codereview.adblockplus.org/29643562/diff/29661558/stylelint-config-ey... File stylelint-config-eyeo/css-properties-order.js (right): https://codereview.adblockplus.org/29643562/diff/29661558/stylelint-config-ey... stylelint-config-eyeo/css-properties-order.js:21: // Display NIT: Looks like this should have a newline before
Thanks Julian! https://codereview.adblockplus.org/29643562/diff/29661558/stylelint-config-ey... File stylelint-config-eyeo/css-properties-order.js (right): https://codereview.adblockplus.org/29643562/diff/29661558/stylelint-config-ey... stylelint-config-eyeo/css-properties-order.js:21: // Display On 2018/01/15 17:51:58, juliandoucette wrote: > NIT: Looks like this should have a newline before I think the newline before is not applicable to the *first* comment. See the rules block in the index.js file.
@Thomas I've change the properties ordering to flexible
On 2018/01/23 13:31:00, ire wrote: > @Thomas I've change the properties ordering to flexible Thanks, I appreciate it. LGTM
@Tristan or @Sebastian please could one of you take a look at this? I've gotten LGTM on the content of the changes
Hey Ire, sorry for the late reply. I only noted some whitespace-errors, please see below. Cheers! https://codereview.adblockplus.org/29643562/diff/29677653/stylelint-config-ey... File stylelint-config-eyeo/css-properties-order.js (right): https://codereview.adblockplus.org/29643562/diff/29677653/stylelint-config-ey... stylelint-config-eyeo/css-properties-order.js:40: This line triggers a "trailing whitespace". https://codereview.adblockplus.org/29643562/diff/29677653/stylelint-config-ey... stylelint-config-eyeo/css-properties-order.js:113: This line triggers a "trailing whitespace". https://codereview.adblockplus.org/29643562/diff/29677653/stylelint-config-ey... stylelint-config-eyeo/css-properties-order.js:182: This line triggers a "trailing whitespace".
On 2018/02/08 11:51:48, tlucas wrote: > Hey Ire, > > sorry for the late reply. > I only noted some whitespace-errors, please see below. > > Cheers! No problem! I've fixed the errors https://codereview.adblockplus.org/29643562/diff/29677653/stylelint-config-ey... File stylelint-config-eyeo/css-properties-order.js (right): https://codereview.adblockplus.org/29643562/diff/29677653/stylelint-config-ey... stylelint-config-eyeo/css-properties-order.js:40: On 2018/02/08 11:51:47, tlucas wrote: > This line triggers a "trailing whitespace". Done. https://codereview.adblockplus.org/29643562/diff/29677653/stylelint-config-ey... stylelint-config-eyeo/css-properties-order.js:113: On 2018/02/08 11:51:47, tlucas wrote: > This line triggers a "trailing whitespace". Done. https://codereview.adblockplus.org/29643562/diff/29677653/stylelint-config-ey... stylelint-config-eyeo/css-properties-order.js:182: On 2018/02/08 11:51:47, tlucas wrote: > This line triggers a "trailing whitespace". Done.
On 2018/02/08 14:12:08, ire wrote: > On 2018/02/08 11:51:48, tlucas wrote: > > Hey Ire, > > > > sorry for the late reply. > > I only noted some whitespace-errors, please see below. > > > > Cheers! > > No problem! I've fixed the errors > > https://codereview.adblockplus.org/29643562/diff/29677653/stylelint-config-ey... > File stylelint-config-eyeo/css-properties-order.js (right): > > https://codereview.adblockplus.org/29643562/diff/29677653/stylelint-config-ey... > stylelint-config-eyeo/css-properties-order.js:40: > On 2018/02/08 11:51:47, tlucas wrote: > > This line triggers a "trailing whitespace". > > Done. > > https://codereview.adblockplus.org/29643562/diff/29677653/stylelint-config-ey... > stylelint-config-eyeo/css-properties-order.js:113: > On 2018/02/08 11:51:47, tlucas wrote: > > This line triggers a "trailing whitespace". > > Done. > > https://codereview.adblockplus.org/29643562/diff/29677653/stylelint-config-ey... > stylelint-config-eyeo/css-properties-order.js:182: > On 2018/02/08 11:51:47, tlucas wrote: > > This line triggers a "trailing whitespace". > > Done. LGTM. Export me the patch and i'll push it :) 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
