Problem/Motivation
Follow-up to #2865971: Use stylelint as opposed to csslint in core. Configure the function-whitespace-after
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
Comment | File | Size | Author |
---|---|---|---|
#32 | reroll_29-32.txt | 669 bytes | Tanuj. |
#32 | 2866815-32.patch | 884 bytes | Tanuj. |
| |||
#29 | 2866815-29.patch | 892 bytes | _utsavsharma |
| |||
#27 | 2866815-27.patch | 1.45 KB | _utsavsharma |
#14 | 2866815-14.patch | 1.5 KB | msankhala |
Comments
Comment #2
alexpottComment #3
Mukeysh CreditAttribution: Mukeysh as a volunteer and at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commentedAdded patch for this.
Comment #4
martin107 CreditAttribution: martin107 as a volunteer commentedI like the patch,
after reading
https://stylelint.io/user-guide/rules/function-whitespace-after/
everything looks correct
+1 from me.
Comment #5
lauriiiWe should update the issue summary with the rule this follows in the coding standards
Comment #7
joelpittetYeah this is a bit suspicious of a rule to me too. Setting to NW for #5
Comment #9
joelpittetI put a ticket in upstream: https://github.com/stylelint/stylelint/issues/3145
Comment #10
joelpittetThey responded quickly and I was reading this incorrectly. CSS concat in content is done with a space and we should have a space on either side.
There is no reference in our standards that match this rule.
I scoured the internet and couldn't find this much but a couple examples that show space on all sides which we should follow:
https://developer.mozilla.org/en-US/docs/Web/CSS/attr
https://davidwalsh.name/css-content-attr
Comment #11
occupantPatch is failing on 8.6.x-dev. Looks like it needs a reroll.
Agreed about the rule configuration @joelpittet outlines in #10 though.
Comment #12
occupantComment #13
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedRerolling patch.
Comment #14
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedRerolled patch uploaded.
Comment #15
occupant#14 applies cleanly, passed linting
Comment #16
alexpott@joelpittet given we have no standards that means we need to create one. The stylelint default seems fine but we should go through the coding standards process - see https://www.drupal.org/project/coding_standards. We also need to update the coding standards pages for stylelint
Comment #17
joelpittetThanks @alexpott I've created that: #2944596: Create a coding standard for CSS to ensure whitespace after function calls
Comment #27
_utsavsharma CreditAttribution: _utsavsharma at OpenSense Labs commentedReroll for 8.7.x
Comment #28
longwaveThis needs reroll for 10.1.x.
Comment #29
_utsavsharma CreditAttribution: _utsavsharma at OpenSense Labs commentedPatch for 10.1.x.
Comment #30
Nikhil_110 CreditAttribution: Nikhil_110 at Srijan | A Material+ Company commentedComment #31
smustgrave CreditAttribution: smustgrave at Mobomo commented#29 needs a reroll.
Comment #32
Tanuj. CreditAttribution: Tanuj. as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedAdding a reroll patch for #29.
Comment #33
Tanuj. CreditAttribution: Tanuj. as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #34
smustgrave CreditAttribution: smustgrave at Mobomo commented#32 applies now.
Comment #35
catchAs far as I can tell this is still blocked on #16/#17 and #2944596: Create a coding standard for CSS to ensure whitespace after function calls.
Marking postponed, but also needs an issue summary update. If we want to commit it before the standard is in place (which I can see doing since it's just a single example in core), that should also be documented in the issue summary.