Problem/Motivation

Follow-up to #2865971: Use stylelint as opposed to csslint in core. Configure the no-empty-source 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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

BrightBold’s picture

Status: Active » Needs work
FileSize
443 bytes

The only code that fails this test is Stark's layout.css, which is completely empty. I wasn't sure what we wanted to do about that, so I left it alone. This tiny patch has nothing except for changing the no-empty-source rule to true.

This patch is dependent on #2865971-41: Use stylelint as opposed to csslint in core.

BrightBold’s picture

Status: Needs work » Active

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

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should 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.

harsha012’s picture

Status: Active » Needs review
FileSize
695 bytes
joelpittet’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

The problem with changing the same file in different patches is that there needs to be a reroll each other one that gets committed.

Checking patch core/.stylelintrc.json...
error: while searching for:
    "function-name-case": null,
    "function-whitespace-after": null,
    "length-zero-no-unit": null,
    "no-empty-source": null,
    "no-unknown-animations": true,
    "number-leading-zero": null,
    "number-no-trailing-zeros": null,

error: patch failed: core/.stylelintrc.json:19
error: core/.stylelintrc.json: patch does not apply
jofitz’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
758 bytes

You know I'm always happy to provide a re-roll.

Status: Needs review » Needs work

The last submitted patch, 8: 2866817-8.patch, failed testing. View results

jofitz’s picture

Version: 8.4.x-dev » 8.5.x-dev
Status: Needs work » Needs review
harsha012’s picture

fixed the patch for 8.4.x

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

Status: Needs review » Reviewed & tested by the community

Thanks @Jo Fitzgerald, @harsha012, @BrightBold, and @alexpott

I'm RTBC #8. I tested the linter with and without the CSS changes and reviewed the changes.

I'm a bit unsure why we have an empty stylesheet and half tempted to recommend removing it but I'll let a committer help decide it's fate.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

#2349711: Remove all visual from stark removed all the CSS from this file. I think we didn't remove it for BC. I think the comment added needs to reflect why it is empty. I didn't fully read the related issue for the reason.

harsha012’s picture

Status: Needs work » Needs review
FileSize
1 KB
1.06 KB

fixed as per the #14 comment

alexpott’s picture

Status: Needs review » Needs work

@harsha012 thanks for the patch. I think we need to add a comment saying it is here for testing purposes - see #2349711-87: Remove all visual from stark

Also there needs to be a new line on the end of the file to comply with coding standards.

harsha012’s picture

Status: Needs work » Needs review
FileSize
892 bytes
807 bytes

fixed as per #16

harsha012’s picture

harsha012’s picture

joelpittet’s picture

Status: Needs review » Needs work

@harsha012 just need a new line and should be made against 8.6.x, it's not a bug so we don't need to backport it. And the line could probably end with "purposes."

jofitz’s picture

Status: Needs work » Needs review
FileSize
865 bytes

Add new line.
Change "purpose" to "purposes".

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 0a1864356c to 8.6.x and f3a5f932a0 to 8.5.x. Thanks!

Adding commit credit for reviewers.

  • alexpott committed 0a18643 on 8.6.x
    Issue #2866817 by harsha012, Jo Fitzgerald, BrightBold, alexpott,...

  • alexpott committed f3a5f93 on 8.5.x
    Issue #2866817 by harsha012, Jo Fitzgerald, BrightBold, alexpott,...

Status: Fixed » Closed (fixed)

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