Problem/Motivation

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

Relevant coding standard from the CSScomb settings for Drupal:

"leading-zero": true,

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
#22 2866818-22.patch30.64 KBharsha012
#17 2866818-17.patch30.63 KBharsha012
#15 interdiff-2866818-13-15.txt338 bytesharsha012
#15 2866818-15.patch19.25 KBharsha012
#13 2866818-13.patch18.86 KBharsha012
#10 interdiff-2866818-7-8.txt10.32 KBbrahmjeet789
#10 stylelint-leading-zero-2866818-8.patch26.96 KBbrahmjeet789
#7 stylelint-leading-zero-2866818-7.patch27.38 KBBrightBold
#4 stylelint-leading-zero-incl-dependency-2866818-2.patch64.5 KBBrightBold
#3 stylelint-leading-zero-2866818-1.patch27.38 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
Parent issue: #2866817: Update stylelint rule no-empty-source 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
FileSize
stylelint-leading-zero-2866818-1.patch27.38 KB

Removed the number-leading-zero rule from .stylelintrc.json since it matches stylelint-config-standard (per #2866816-9: Update stylelint rule length-zero-no-unit to be consistent with Drupal's CSS standards, thanks idebr) and updated all CSS to pass.

This patch relies on #2865971-44: Use stylelint as opposed to csslint in core so won't pass testing until that is committed.

BrightBold’s picture

BrightBold
she/her
Primary language English
Location Boston, MA
CreditAttribution: BrightBold commented
Status: Active » Needs review
FileSize
stylelint-leading-zero-incl-dependency-2866818-2.patch64.5 KB

Patch for testing purposes which combines the patch in #1 above, plus the patch from #2865971-44: Use stylelint as opposed to csslint in core.

droplet’s picture

droplet CreditAttribution: droplet commented
Version: 8.3.x-dev » 8.4.x-dev
Status: Needs review » Needs work

Needs reroll

BrightBold’s picture

BrightBold
she/her
Primary language English
Location Boston, MA
CreditAttribution: BrightBold commented
Assigned: Unassigned » BrightBold
BrightBold’s picture

BrightBold
she/her
Primary language English
Location Boston, MA
CreditAttribution: BrightBold commented
FileSize
stylelint-leading-zero-2866818-7.patch27.38 KB

This is the same as the patch in #1, which seemed like it still applied so let's re-test with 8.4.

BrightBold’s picture

BrightBold
she/her
Primary language English
Location Boston, MA
CreditAttribution: BrightBold commented
Assigned: BrightBold » Unassigned
BrightBold’s picture

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

brahmjeet789 CreditAttribution: brahmjeet789 as a volunteer and at gai Technologies Pvt Ltd commented
FileSize
stylelint-leading-zero-2866818-8.patch26.96 KB
interdiff-2866818-7-8.txt10.32 KB

Hi BrightBold,

i have changed some things in ur patch may be it works .

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 change and outside_in doesn't exist in the same space, this needs a reroll

error: core/modules/outside_in/css/off-canvas.motion.css: does not exist in index
Checking patch core/modules/outside_in/css/outside_in.details.css...
error: core/modules/outside_in/css/outside_in.details.css: does not exist in index
Checking patch core/modules/outside_in/css/outside_in.form.css...
error: core/modules/outside_in/css/outside_in.form.css: does not exist in index
Checking patch core/modules/outside_in/css/outside_in.motion.css...
error: core/modules/outside_in/css/outside_in.motion.css: does not exist in index
Checking patch core/modules/outside_in/css/outside_in.table.css...
error: core/modules/outside_in/css/outside_in.table.css: does not exist in index
Checking patch core/modules/outside_in/css/outside_in.tabledrag.css...
error: core/modules/outside_in/css/outside_in.tabledrag.css: does not exist in index
Checking patch core/modules/outside_in/css/outside_in.theme.css...
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
2866818-13.patch18.86 KB
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

Thanks @harsha012, although the core/.stylelintrc.json looks like it got removed from the last patch, could you put it back?

harsha012’s picture

harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commented
Status: Needs work » Needs review
FileSize
2866818-15.patch19.25 KB
interdiff-2866818-13-15.txt338 bytes
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

Looks like this is conflicting with some of the other CSS commits: needs another reroll.

patching file core/.stylelintrc.json
Hunk #1 succeeded at 14 (offset -2 lines).
patching file core/modules/ckeditor/css/ckeditor.admin.css
Hunk #1 FAILED at 180.
1 out of 1 hunk FAILED -- saving rejects to file core/modules/ckeditor/css/ckeditor.admin.css.rej
Hunk #1 FAILED at 180.
1 out of 1 hunk FAILED -- saving rejects to file core/themes/stable/css/ckeditor/ckeditor.admin.css.rej
harsha012’s picture

harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commented
Status: Needs work » Needs review
FileSize
2866818-17.patch30.63 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
Issue tags: -Needs reroll

This looks like it doesn't match the CSSComb rule in the issue summary, I just noticed where we do have a leading 0. I personally find it easier to scan for fractions with a leading 0.

Could I recommend we match the CSSComb, or are there strong feelings for this one? Also it's a big patch but there would be no CSS changes if we turn this to true as an added bonus.

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

No big objections, changing to needs work to see if any takers?

harsha012’s picture

harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commented

@joelpittet,

Since we are setting rule "number-leading-zero": true for adding a leading zero. if we see the css combo setting https://github.com/csscomb/csscomb.js/blob/master/doc/options.md#leading..., to add the leading zero we need to set true. Is there any specific change you are looking in core/.stylelintrc.json.

Please advice.

joelpittet’s picture

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

I’m proposing we set it to true and keep the leading 0s

harsha012’s picture

harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commented
Status: Needs work » Needs review
FileSize
2866818-22.patch30.64 KB

fixed as per comment #18,

set the rule "number-leading-zero": "always"

getting error on setting rule
"number-leading-zero": true

Invalid Option: Invalid option value "true" for rule "number-leading-zero"

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

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.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 » Reviewed & tested by the community

Interesting that the patches are the same size:) This feels easily bikeshedable, kind of wish I had another person agree with the direction, but for me this RTBC.

Thanks @harsha012, @BrightBold, @brahmjeet789, and @alexpott.

I reviewed with and without the changes running the linter to ensure it's catching everything and used git diff --color-words to scan the code changes.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 22: 2866818-22.patch, failed testing. View results

joelpittet’s picture

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

Putting this back to RTBC. Still applies and the fail looks unrelated.

alexpott’s picture

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

Looking at our CSS standards page and existing CSS comb rule this seems reasonable.

Committed and pushed f0272d5fa6 to 8.6.x and c2f2111781 to 8.5.x. Thanks!

  • alexpott committed f0272d5 on 8.6.x 
    Issue #2866818 by harsha012, BrightBold, brahmjeet789, joelpittet:...

Status: Fixed » Closed (fixed)

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