Problem/Motivation

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

From the CSS Formatting Guidelines:

Include a space after each comma in comma-separated property or function values.

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
#14 stylelint-declaration-colon-space-vendor-prefix-incl-2866809-14.patch14.49 KBBrightBold
#10 stylelint-declaration-colon-space-vendor-prefix-passes-incl-2865971-41-2866809-10.patch55.93 KBBrightBold
#9 stylelint-declaration-colon-space-vendor-prefix-passes-2866809-8.patch14.73 KBBrightBold
#7 stylelint-declaration-colon-space-vendor-prefix-passes-incl-2865971-41-2866809-7.patch55.93 KBBrightBold
#6 stylelint-declaration-colon-space-vendor-prefix-passes-2866809-6.patch14.73 KBBrightBold
#5 stylelint-declaration-colon-space-with-vendor-prefix-exceptions-incl-2865971-41-2866809-5.patch49.52 KBBrightBold
#4 stylelint-declaration-colon-space-with-vendor-prefix-exceptions-2866809-4.patch7.33 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: #2866808: Update stylelint rule declaration-block-trailing-semicolon 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
Assigned: Unassigned » BrightBold
BrightBold’s picture

BrightBold
she/her
Primary language English
Location Boston, MA
CreditAttribution: BrightBold commented
FileSize
stylelint-declaration-colon-space-with-vendor-prefix-exceptions-2866809-4.patch7.33 KB

This patch depends on the patch from #2865971-41: Use stylelint as opposed to csslint in core.

Also, I noticed there were a section in the CSS Formatting Guidelines for "exceptions and slight deviations." Formatting of vendor-prefixed properties wasn't covered there, but frequently in the CSS people had those values right-aligned. I wasn't sure whether that was an omission in the coding standard or whether we wanted to change them all to pass this lint condition, so I'll do one patch each way. This patch assumes that vendor prefixes are an exception to the colon-space rule.

BrightBold’s picture

BrightBold
she/her
Primary language English
Location Boston, MA
CreditAttribution: BrightBold commented
FileSize
stylelint-declaration-colon-space-with-vendor-prefix-exceptions-incl-2865971-41-2866809-5.patch49.52 KB

Same as above but also includes the patch from #2865971-41: Use stylelint as opposed to csslint in core, because the patch above will fail testing since it's dependent on stuff that hasn't been committed.

BrightBold’s picture

BrightBold
she/her
Primary language English
Location Boston, MA
CreditAttribution: BrightBold commented
FileSize
stylelint-declaration-colon-space-vendor-prefix-passes-2866809-6.patch14.73 KB

This patch depends on the patch from #2865971-41: Use stylelint as opposed to csslint in core and as such will not pass tests until that patch is committed.

It assumes we don't want special alignment for vendor prefixes and those have now all been fixed to comply to the declaration-colon-space rule.

There are three stylesheets that still generate linting errors because they fall under this exception:

Long, comma-separated property values—such as collections of gradients or shadows—can be arranged across multiple lines in an effort to improve readability and produce more useful diffs.

Those three stylesheets are:

  • themes/classy/css/components/progress.css
  • themes/seven/css/components/form.css
  • themes/seven/css/theme/install-page.css
BrightBold’s picture

BrightBold
she/her
Primary language English
Location Boston, MA
CreditAttribution: BrightBold commented
FileSize
stylelint-declaration-colon-space-vendor-prefix-passes-incl-2865971-41-2866809-7.patch55.93 KB

Same as patch in #6 but includes the patch from #2865971-41: Use stylelint as opposed to csslint in core so it will (hopefully!) pass testing.

BrightBold’s picture

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

BrightBold
she/her
Primary language English
Location Boston, MA
CreditAttribution: BrightBold commented
FileSize
stylelint-declaration-colon-space-vendor-prefix-passes-2866809-8.patch14.73 KB

Aargh this replaces the patch in #6 because d'oh.

BrightBold’s picture

BrightBold
she/her
Primary language English
Location Boston, MA
CreditAttribution: BrightBold commented
FileSize
stylelint-declaration-colon-space-vendor-prefix-passes-incl-2865971-41-2866809-10.patch55.93 KB

This replaces the patch in #7. *headdesk*

The last submitted patch, 4: stylelint-declaration-colon-space-with-vendor-prefix-exceptions-2866809-4.patch, failed testing.

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
Version: 8.3.x-dev » 8.4.x-dev
Assigned: Unassigned » BrightBold
BrightBold’s picture

BrightBold
she/her
Primary language English
Location Boston, MA
CreditAttribution: BrightBold commented
FileSize
stylelint-declaration-colon-space-vendor-prefix-incl-2866809-14.patch14.49 KB

Rerolled for 8.4.x, with the assumption we don't want to make an exception for vendor prefixes. (If I guessed that wrong I'll redo it.)

BrightBold’s picture

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

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

Giving this a try it looks like it needs a re-roll. The patch looks on target though, thanks for making this @BrightBold

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
Issue tags: -Needs reroll

Actually my bad, applied the patch with the wrong command in the wrong folder. It applies, and I ran the linter with no errors, reran with one of the files reverted to ensure the rule still picked up the spaces and it does.

alexpott’s picture

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

Committed c5654de and pushed to 8.5.x. Thanks!

  • alexpott committed c5654de on 8.5.x 
    Issue #2866809 by BrightBold: Update stylelint rule declaration-colon-...

Status: Fixed » Closed (fixed)

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