Problem/Motivation

Follow-up to #2865971: Use stylelint as opposed to csslint in core. Configure the length-zero-no-unit to be consistent with https://www.drupal.org/docs/develop/standards/css/css-coding-standards

From the CSS Formatting Guidelines:

Where allowed, avoid specifying units for zero-values, e.g. use margin: 0; instead of margin: 0px;.

Proposed resolution

Brief instructions on running stylelint - you'll need npm...

All the commands below take place in DRUPAL_ROOT/core
To install stylelint

npm install

This will install Drupal 8's npm dependencies of which stylelint is one.

To run it on all core css files. Apply this issue's patch and do the following command from DRUPAL_ROOT/core

npm run lint:css

Remaining tasks

User interface changes

None

API changes

None

CommentFileSizeAuthor
#23 interdiff-2866816-21-22.txt654 bytesharsha012
#23 2866816-22.patch6.72 KBharsha012
#21 interdiff-2866816-19-21.txt985 bytesharsha012
#21 2866816-21.patch6.9 KBharsha012
#19 2866816-19.patch6.25 KBharsha012
#13 2866816-length_0_no_unit_lint-13.patch7.6 KBchrisfromredfin
#6 stylelint-length-zero-no-unit-with-dependency-2866816-6.patch48.26 KBBrightBold
#5 stylelint-length-zero-no-unit-2866816-5.patch7.57 KBBrightBold
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Parent issue: #2866815: Update stylelint rule function-whitespace-after to be consistent with Drupal's CSS standards » #2865971: Use stylelint as opposed to csslint in core
BrightBold’s picture

BrightBold
she/her
Primary language English
Location Boston, MA
CreditAttribution: BrightBold commented
Issue summary: View changes
BrightBold’s picture

BrightBold
she/her
Primary language English
Location Boston, MA
CreditAttribution: BrightBold commented
FileSize
stylelint-length-zero-no-unit-2866816-5.patch7.57 KB

Patch with length-zero-no-unit rule and associated CSS fixes.

This patch is dependent on #2865971-41: Use stylelint as opposed to csslint in core so will fail testing until that one is committed.

BrightBold’s picture

BrightBold
she/her
Primary language English
Location Boston, MA
CreditAttribution: BrightBold commented
FileSize
stylelint-length-zero-no-unit-with-dependency-2866816-6.patch48.26 KB

Patch for testing purposes that combines the patch in #5 above with the patch in #2865971-41: Use stylelint as opposed to csslint in core.

BrightBold’s picture

BrightBold
she/her
Primary language English
Location Boston, MA
CreditAttribution: BrightBold commented
Status: Active » Needs review
idebr’s picture

idebr CreditAttribution: idebr at ezCompany commented
Status: Needs review » Needs work

Hi BrightBold,

Thanks for working on this issue! The patch works as expected, however I would like to suggest one change:

+++ b/core/.stylelintrc.json
@@ -18,7 +18,7 @@
-    "length-zero-no-unit": null,
+    "length-zero-no-unit": true,

Considering this line matches the "stylelint-config-standard" we are extending, I suggest we remove this line completely.

Source: https://github.com/stylelint/stylelint-config-standard/blob/master/index...

BrightBold’s picture

BrightBold
she/her
Primary language English
Location Boston, MA
CreditAttribution: BrightBold commented

Oh, good point idebr.

However, will this make nearly every other stylelint-related patch (I've made a bunch so far) have to be rerolled each time we apply one because the linenumbers in .stylelintrc.json will have changed? If so, is it more efficient to keep setting the values and then once all the patches are committed, go back and remove all the rules that duplicate stylelint-config-standards? Or is it better to go ahead with removing redundant lines so each issue is completely solved, and just know we will be rerolling tons of patches? (I'm assuming some will have to be rerolled anyway when errors for different rules appear in the same line of code.) Or is git smarter than I think and I need to stop worrying?

idebr’s picture

idebr CreditAttribution: idebr at ezCompany commented

However, will this make nearly every other stylelint-related patch (I've made a bunch so far) have to be rerolled each time we apply one because the linenumbers in .stylelintrc.json will have changed?

Considering we are not changing the same lines, subsequent patches would probably not have to be rerolled. However, I agree we should create a followup if it turns out rerolling the patches turns out to be a lot of work.

BrightBold’s picture

BrightBold
she/her
Primary language English
Location Boston, MA
CreditAttribution: BrightBold commented

Sounds like a plan @idebr. It will probably be a few days before I can get back to working on these, so in the meantime I will keep my eye on #2865971: Use stylelint as opposed to csslint in core. Once it's resolved & committed and one of the child issues is reviewed & committed I can see how much trouble rerolling will be and make a call on how to proceed.

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
CreditAttribution: chrisfromredfin at Redfin Solutions, LLC commented
Assigned: Unassigned » chrisfromredfin
chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
CreditAttribution: chrisfromredfin at Redfin Solutions, LLC commented
Version: 8.3.x-dev » 8.4.x-dev
Assigned: chrisfromredfin » Unassigned
Status: Needs work » Needs review
FileSize
2866816-length_0_no_unit_lint-13.patch7.6 KB

Updated against 8.4.x

Status: Needs review » Needs work

The last submitted patch, 13: 2866816-length_0_no_unit_lint-13.patch, failed testing. View results

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
CreditAttribution: chrisfromredfin at Redfin Solutions, LLC commented
Status: Needs work » Needs review

Test only ran against 8.3 by accident. 8.4 passes, so setting back to NR.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer and at The University of British Columbia commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

One more snuck in 🙃

themes/seven/css/components/form.css
 244:5  ✖  Expected empty line before declaration   declaration-empty-line-before
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer and at The University of British Columbia commented

Whoops, that was from an upgrade test for stylelint.

The actual reroll is because this file doesn't exist

error: core/modules/outside_in/css/outside_in.theme.css: does not exist in index

harsha012’s picture

harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commented
Status: Needs work » Needs review
FileSize
2866816-19.patch6.25 KB

re-rolled the patch

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer and at The University of British Columbia commented
Status: Needs review » Needs work

One more change snuck in... 🙃

modules/content_moderation/css/content_moderation.theme.css
 7:16  ✖  Unexpected unit   length-zero-no-unit
harsha012’s picture

harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commented
Status: Needs work » Needs review
FileSize
2866816-21.patch6.9 KB
interdiff-2866816-19-21.txt985 bytes

fixed as per #20

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs review » Needs work 
+++ b/core/modules/simpletest/css/simpletest.module.css
@@ -1,4 +1,3 @@
-

+++ b/core/themes/stable/css/simpletest/simpletest.module.css
@@ -1,4 +1,3 @@
-

Unrelated changes.

harsha012’s picture

harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commented
Status: Needs work » Needs review
FileSize
2866816-22.patch6.72 KB
interdiff-2866816-21-22.txt654 bytes

removed the unrelated changes

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer and at The University of British Columbia commented
Status: Needs review » Reviewed & tested by the community

Used git diff --color-words to see if unrelated changes were there. Ran the linter with and without the CSS changes to ensure they are caught by the rule.

Thanks @harsha012, @cswells, and @alexpott!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 7bf384b and pushed to 8.5.x. Thanks!

  • alexpott committed 7bf384b on 8.5.x 
    Issue #2866816 by harsha012, BrightBold, cwells, joelpittet, idebr:...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.