Problem/Motivation

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

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
#15 2866804-15.patch2.99 KBjoelpittet
#11 interdiff-2866804-9-11.txt1.37 KBharsha012
#11 2866804-11.patch1.87 KBharsha012
#9 2866804-9.patch1.95 KBharsha012
#6 2866804-duplicate-properties-6.patch2.81 KBBrightBold
#3 2866804-duplicate-properties-3.patch2.81 KBcsd713
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: #2866803: Update stylelint rule comment-empty-line-before to be consistent with Drupal's CSS standards » #2865971: Use stylelint as opposed to csslint in core
csd713’s picture

csd713 CreditAttribution: csd713 at Acquia commented
Status: Active » Needs review
FileSize
2866804-duplicate-properties-3.patch2.81 KB

I have removed few IE-8 only supported properties. one example is below where 'background-color: silver' has been removed.

(modules/ckeditor/css/ckeditor.admin.css)

background-color: silver;
background-color: rgba(0, 0, 0, 0.2)

I also have run the stylelint with below rule.

"declaration-block-no-duplicate-properties": [true, {
      "ignore": "consecutive-duplicates"
    }],

Status: Needs review » Needs work

The last submitted patch, 3: 2866804-duplicate-properties-3.patch, failed testing. View results

BrightBold’s picture

BrightBold
she/her
Primary language English
Location Boston, MA
CreditAttribution: BrightBold commented
Version: 8.3.x-dev » 8.4.x-dev
BrightBold’s picture

BrightBold
she/her
Primary language English
Location Boston, MA
CreditAttribution: BrightBold commented
Status: Needs work » Needs review
FileSize
2866804-duplicate-properties-6.patch2.81 KB

Reuploading @csd713's patch from #3 to re-test against 8.4.x, because I'm guessing that's why it didn't apply. (Note: this is the exact same patch; I just couldn't figure out how to get it to retest against a different branch of Drupal without re-uploading.)

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

This one didn't pickup the duplicate in core/modules/shortcut/css/shortcut.theme.css Maybe that change isn't needed. Setting to Needs Work to see what you think.

harsha012’s picture

harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commented
Status: Needs work » Needs review
FileSize
2866804-9.patch1.95 KB

please check

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

The first values need to be removed not the second ones because in CSS the second one will override the first one and if we remove the second one we will inadvertently change the CSS style.

harsha012’s picture

harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commented
Status: Needs work » Needs review
FileSize
2866804-11.patch1.87 KB
interdiff-2866804-9-11.txt1.37 KB

fixed

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

Thats better, thanks @harsha012, @BrightBold, @csd713 and @alexpott!

I double checked the 3 hunks and tested the the change with and without the CSS changes to ensure the linter picked up the changes.

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.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work 
yarn run v1.1.0
$ stylelint "**/*.css" || exit 0

modules/toolbar/css/toolbar.module.css
 250:3  ✖  Unexpected duplicate "auto"   declaration-block-no-duplicate-properties

themes/classy/css/components/progress.css
 22:3  ✖  Unexpected duplicate "-webkit-linear-gradient(top, rgba(0, 0, 0, 0), rgba(0, 0, 0, 0.15)),                                 declaration-block-no-duplicate-properties
          -webkit-linear-gradient(left top, #0094f0:0%, #0094f0 25%, #007ecc 25%, #007ecc 50%, #0094f0 50%, #0094f0 75%, #0094f0
          100%)"

themes/stable/css/toolbar/toolbar.module.css
 250:3  ✖  Unexpected duplicate "auto"   declaration-block-no-duplicate-properties

Removing the "ignore": "consecutive-duplicates" brings up these. Are we sure that we want to ignore these?

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 » Needs review
FileSize
2866804-15.patch2.99 KB

I think that was misinterpreted originally, it only seems to remove exact duplicates so it doesn't remove browser compatibility differences only exact duplicates. So yes we should remove that.

Here's a reroll.
There is a regression for umami that snuck in that I didn't fix here because it was fixed in the update stylelint version issue.
profiles/demo_umami/themes/umami/css/components/blocks/banner/banner.css

occupant’s picture

occupant
Primary language English
Location Canada
CreditAttribution: occupant at The University of British Columbia commented
Status: Needs review » Reviewed & tested by the community

#15 applies cleanly. 

npm run lint:css                                              8.6.x  ✭ ✱

> Drupal@ lint:css /mypath/drupal/core
> stylelint "**/*.css" || exit 0

The 3 umami errors for the function-comma-space-after rule are there, but @joelpittet has already addressed those.

alexpott’s picture

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

Committed 93f9320 and pushed to 8.6.x. Thanks!

  • alexpott committed 93f9320 on 8.6.x 
    Issue #2866804 by harsha012, joelpittet, BrightBold, csd713: Update...

Status: Fixed » Closed (fixed)

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