Problem/Motivation

We have CSS standards now. There's a tool called CSSLint that helps front-end devs write better CSS.

There are some rules in CSSLint that are not inline with our CSS standards. See #1190252: [573] Use csslint as a weapon to beat the crappy CSS out of Drupal core. We can make this tool better by deciding which rules to include when CSSLint is run against Drupal core.

Proposed resolution

Include a .csslintrc file in Drupal core.

Remaining tasks

  1. Decide which rules to include (true or false)
  2. Write a patch

Find rule documentation here:
https://github.com/CSSLint/csslint/wiki/Rules

Suggested rules

  • important - true
  • adjoining-classes - false
  • known-properties - true
  • box-sizing - false
  • box-model - true
  • overqualified-elements - true
  • display-property-grouping - true
  • bulletproof-font-face - false
  • compatible-vendor-prefixes - false
  • regex-selectors - false
  • errors (???)
  • duplicate-background-images - true
  • duplicate-properties - true
  • empty-rules - true
  • selector-max-approaching (???)
  • gradients - false
  • fallback-colors - false
  • font-sizes - false
  • floats - false
  • star-property-hack - true
  • outline-none - true
  • import - true
  • ids - true
  • underscore-property-hack - true
  • qualified-headings - true
  • selector-max (??)
  • shorthand - true
  • text-indent - true
  • unique-headings - true
  • universal-selector - false
  • unqualified-attributes - true
  • vendor-prefix - true
  • zero-units - true
CommentFileSizeAuthor
#15 drupal-add_csslint_file-2222049-13.patch1.09 KBlewisnyman
#15 test-gruntfile-2222049-do-not-test.patch1.42 KBlewisnyman
#15 csslint-222049-13.txt15.63 KBlewisnyman
#15 csslint-HEAD.txt26.84 KBlewisnyman
#13 interdiff-2222049-7-13.txt1.61 KBMichael Hodge Jr
#13 drupal-add_csslint_file-2222049-13-do-not-test.patch1.09 KBMichael Hodge Jr
#7 issue-2222049-add-csslintrc-file-do-not-test.patch1.11 KBsqndr

Comments

lewisnyman’s picture

lewisnyman
he/him
commented
Title: Add a .csslintrc file that's inline with our CSS standards » Add a .csslintrc file that's in line with our CSS standards
lewisnyman’s picture

lewisnyman
he/him
commented
Issue summary: View changes

I've added a list of all default rules to the issue summary. We can go through each one and decide if they are off, warning, or error. We can even write our own tests later :)

mgifford’s picture

mgifford
he/him
Primary language English
commented

This sounds like a really amazing idea. Could go a long ways to help manage all the CSS files in Core and later in Contrib.

yesct’s picture

yesct commented
Issue tags: +css coding standards, +Coding standards
sqndr’s picture

sqndr commented

I've created a .csslintrc file with all the rules above as false as a default for now.

.csslintrc:

{
  "important": false,
  "adjoining-classes": false,
  "known-properties": false,
  "box-sizing": false,
  "box-model": false,
  "overqualified-elements": false,
  "display-property-grouping": false,
  "bulletproof-font-face": false,
  "compatible-vendor-prefixes": false,
  "regex-selectors": false,
  "errors": false,
  "duplicate-background-images": false,
  "duplicate-properties": false,
  "empty-rules": false,
  "selector-max-approaching": false,
  "gradients": false,
  "fallback-colors": false,
  "font-sizes": false,
  "font-faces": false,
  "floats": false,
  "star-property-hack": false,
  "outline-none": false,
  "import": false,
  "ids": false,
  "underscore-property-hack": false,
  "rules-count": false,
  "qualified-headings": false,
  "selector-max": false,
  "shorthand": false,
  "text-indent": false,
  "unique-headings": false,
  "universal-selector": false,
  "unqualified-attributes": false,
  "vendor-prefix": false,
  "zero-units": false
}

I did not create a patch yet, since I have no idea where this file should go? Any suggestions on a good location? Anyway, seems like we also need to sort out what rules are important for the Drupal css. Let's get this discussion going!

Also ... Once the file is in core, I feel like we need some really good documentation on how to use this!

Anonymous’s picture

Anonymous (not verified) commented

You just need to create the patch in the root folder, alongside .jshintrc (which will soon become .eslintrc, but that's by the by).

Re: documentation, definitely. There's a lot we could wiki-fy for frontend tooling.

sqndr’s picture

sqndr commented
StatusFileSize
new issue-2222049-add-csslintrc-file-do-not-test.patch1.11 KB

Right, here's a first patch. Again - this is with all rules set to false at the moment.

lewisnyman’s picture

lewisnyman
he/him
commented
Issue tags: -frontend performance +frontend
lewisnyman’s picture

lewisnyman
he/him
commented
Issue summary: View changes

I've updated the summary and marked true/false against all of the rules. You can view the documentation for every rule here:
https://github.com/CSSLint/csslint/wiki/Rules

I think we're ready to commit this. If someone strongly disagrees with one of these rules they are easy to change. It would be great to make CSS lint a formal part of our review process.

sqndr’s picture

sqndr commented

The things left to do would be to:
- Create a patch
- Update the documentation.

sqndr’s picture

sqndr commented
Issue tags: +Novice

I'll mark this as novice. Creating the patch seems like a novice tasks.

Michael Hodge Jr’s picture

Michael Hodge Jr commented
Assigned: Unassigned » Michael Hodge Jr
Michael Hodge Jr’s picture

Michael Hodge Jr commented
Status: Active » Needs review
StatusFileSize
new drupal-add_csslint_file-2222049-13-do-not-test.patch1.09 KB
new interdiff-2222049-7-13.txt1.61 KB

I went ahead and applied the patch in #7 against HEAD. I then modified the .csslintrc to match the statuses in the issue summary. Attached is that patch and the interdiff.txt.

Michael Hodge Jr’s picture

Michael Hodge Jr commented
Assigned: Michael Hodge Jr » Unassigned
lewisnyman’s picture

lewisnyman
he/him
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new csslint-HEAD.txt26.84 KB
new csslint-222049-13.txt15.63 KB
new test-gruntfile-2222049-do-not-test.patch1.42 KB
new drupal-add_csslint_file-2222049-13.patch1.09 KB

It seems like the current npm version of csslint (0.10.0) does not support json .csslintrc files but 0.10.1 does.

I had to create a small gruntfile to test this, as you can pull in the options as json in the csslint grunt task
I've attached the output of the grunt task with and without this patch, the diff between files wasn't that helpful but it's clear just from the file sizes alone that it's picking up our options.

I don't there's anything else to do here, I've attached a patch that contains the grunt task if anyone else wants to test. Finally, I've reuploaded the patch from #13 but without 'do-not-test' so we can get some green and RTBC.

Also, I found a really good example file: https://github.com/birkestroem/plopjs/blob/master/images/csslint.json

alexpott’s picture

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

So yay +1 to this. It is such a shame that the csslint shipped by npm does not (yet) support the json rc file. Having to create a local grunt project to test this is a big -- and will make integration into my pre commit hooks unnecessarily complex.

I think this patch should also fix all the errors. Since there is no point having an automated standard that we are not applying.

lewisnyman’s picture

lewisnyman
he/him
commented
Status: Needs work » Reviewed & tested by the community

Thanks Alex, we already have a meta to fix csslint errors #1190252: [573] Use csslint as a weapon to beat the crappy CSS out of Drupal core. I think this issue helps make that meta more achievable. It feels impossible to fix all the csslint warnings in one issue, and accurately testing for visual regressions across the whole of core, I can't see this issue ever being committed with that approach.

I'm happy to get a few more opinions in here.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: +revisit before release candidate

Okay - we can always remove this before release if we don't get it done.

Committed 5f37ebc and pushed to 8.0.x. Thanks!

  • alexpott committed 5f37ebc on 8.0.x 
    Issue #2222049 by LewisNyman, Michael Hodge Jr, sqndr: Add a .csslintrc...

Status: Fixed » Closed (fixed)

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue tags: -revisit before release candidate

Discussed with @xjm, @catch, @effulgentsia and @webchick - we decided that it was okay to leave in core even if core does not comply. Removing tag.