Skip to:
Content

BuddyPress.org

Opened 4 years ago

Last modified 3 years ago

#7028 closed enhancement

Use stylelint to lint SCSS & CSS replacing Ruby Gem `scss_lint` — at Version 13

Reported by: netweb Owned by:
Milestone: 2.9 Priority: normal
Severity: normal Version: 2.3.0
Component: Build/Test Tools Keywords: needs-patch needs-refresh
Cc:

Description (last modified by netweb)

BuddyPress currently uses a Ruby gem scss_lint, this has always been difficult to maintain and complicates the setup required to contribute to BuddyPress. This ticket replaces that Ruby gem with a NodeJS module stylelint

The base configuration stylelint-config-wordpress was created to adhere to WordPress' CSS coding standards, this also includes SCSS in the same set of coding standards.

stylelint yes, lowercase_s_dangit is a PostCSS plugin for linting CSS, SCSS, Less, SugarSS files.

"Enforce consistent conventions and avoid errors in your stylesheets with stylelint, a modern CSS linter. "

See also: #bbPress2924 #WP29792

Change History (14)

@netweb
4 years ago

#1 @netweb
4 years ago

In 7028.patch apart from being a whopping 289.0 KB is the 1st pass at linting all of BP's SCSS & CSS to adhere to WordPress Coding Standrds.

ToDo: Primarily the only issue here is to update the rules for when and where new lines appear using rules:

  • at-rule-empty-line-before
  • block-opening-brace-newline-after
  • block-closing-brace-newline-before
  • rule-nested-empty-line-before

#2 follow-up: @DJPaul
4 years ago

  • Milestone changed from Awaiting Review to Future Release

We don't need the changed CSS files in the patch, only the grunt file, etc. Can you trim the patch down? But thank you for making it easy for us to see what the file changes.

We'll also need to update the Travis-CI configuration because I'm pretty sure we had to customise it to get the old approach working.

It looks like we can commit this as soon as you're happy with however you choose to resolve those TODOs. I figure we're happy to take your advice on this.

Thanks @netweb!

#3 follow-up: @hnla
4 years ago

There may be a few things to consider before committing.
We seem to induce new errors that passed before under scsslint.

Why do those scss lint commands need to be in true css comment syntax as in a scss file that means they are compiled pointlessly to the real stylefile, with scsslint I used their rules but in forward slash escapes ''

We need to be careful about placing restrictions on how coders use things like shorthand properties they can be an arcane process requiring nuanced use.

block-opening-brace-newline-after
block-closing-brace-newline-before

no!
If it means this:

selector {

 property: value;

}

at-rule-empty-line-before

Yes - think I probably have done this instinctively, if not it can only help legibility to have a clear line.

rule-nested-empty-line-before

Not sure what this one refers to, seems same as one above, but if this is this:

selector {

 nested {
  property: value;
 }
}

We do have a rule presently to cover a newline but after a property/value pair in the parent. If we go straight in with a nested selector with parent having no properties lets do a newline ( probably guilty of not having done so previously ).

#4 in reply to: ↑ 2 @netweb
4 years ago

Replying to DJPaul:

We don't need the changed CSS files in the patch, only the grunt file, etc. Can you trim the patch down? But thank you for making it easy for us to see what the file changes.

The CSS changes are indeed needed in the patch, they will, or should be committed. The changes make BuddyPress' CSS adhere to WordPress CSS coding standards, the new stylelint lint task checks these standards against the CSS going forward.

We'll also need to update the Travis-CI configuration because I'm pretty sure we had to customise it to get the old approach working.

Yes, forgot about this, will include it in the next patch iteration.

It looks like we can commit this as soon as you're happy with however you choose to resolve those TODOs. I figure we're happy to take your advice on this.

Thanks @netweb!

:)

#5 in reply to: ↑ 3 @netweb
4 years ago

Replying to hnla:

There may be a few things to consider before committing.

Indeed, there are quite a few things needed to consider and go through before committing :)

We seem to induce new errors that passed before under scsslint.

The goal here is to adhere to WordPress' CSS coding standards, which includes CSS. stylelint is a far more powerful and granular than that of scss_lint, hence more fine grained issues can be checked using stylelint that scss_lint.

So as much as scss_lint has served BP well up until now there are CSS issues that do not match the coding standards because either scss_lint had no option to detect those issues or the option was not enabled.


Why do those scss lint commands need to be in true css comment syntax as in a scss file that means they are compiled pointlessly to the real stylefile, with scsslint I used their rules but in forward slash escapes ''


Where exactly, can you give me an example of where I missed that? My quick check of the 7028.patch shows the double slash // comments untouched and we should continue to use both comment formats where applicable so that comments we want compiled and to appear in the compiled CSS using /* and to not be compiled with //

We need to be careful about placing restrictions on how coders use things like shorthand properties they can be an arcane process requiring nuanced use.

Yes, the coding standards state: "Use shorthand (except when overriding styles) for background, border, font, list-style, margin, and padding values as much as possible. (For a shorthand reference, see CSS Shorthand."

The stylelint rule shorthand-property-no-redundant-values and declaration-block-no-shorthand-property-overrides rules handle this. Is there a case where this was missed in the CSS?

The new line issues I'll come back to in another comment to avoid cluttering this comment with a huge wall of text ;)

#6 follow-up: @hnla
4 years ago

@netweb

Why do those scss lint commands need to be in true css comment syntax as in a scss file that means they are compiled pointlessly to the real stylefile, with scsslint I used their rules but in forward slash escapes

Where exactly, can you give me an example of where I missed that? My quick check of the 7028.patch​ shows the double slash comments untouched and we should continue to use both comment formats where applicable so that comments we want compiled and to appear in the compiled CSS using /* and to not be compiled with

It was glancing through and seeing in the compiled css files generated from the scss companion sheets:

/* stylelint-disable declaration-colon-space-after */ 
/* stylelint-enable */ 

In the twentyfifteen.scss file we have:

// Variables: color definitions 
 	129	/* stylelint-disable declaration-colon-space-after */ 
127	130	$content-background: #fff; 
128	131	$light-background:   #f7f7f7; 
129	132	$medium-background:  #ccc; 
… 	… 	 
141	144	$stripe-even:        #dbe5ff; 
142	145	$unread:             #dce5ff; 
143	146	$link-action:        #c82b2b; 
 	147	/* stylelint-enable */

Preserving the human readable formatting keeping the hex codes lined up.
I presume we can change the css comments to forward slash (These comments are after all not comments we want compiled to a true stylesheet) as per my example in the sheets where I cancel the check on vendor prefix for a mixins block.

#7 follow-up: @hnla
4 years ago

Yes, the coding standards state: "Use shorthand (except when overriding styles) for background, border, font, list-style, margin, and padding values as much as possible. (For a shorthand reference, see CSS Shorthand."

And how does this automated test determine when styles are attempting to override something that perhaps is located somewhere in the Cascade?

The reference to 'shorthand' actually covers both:

property: 0 0 0;

and

property: 0 0 0 0;

Programatically it is next to impossible to tell why one is used over another and neither is actually incorrect ( this is very similar to an issue that always used to exist in the jigsaw validator).

Aspects like this of CSS coding are tricky and where we always preferred that people would write 0 0 0 especially if obvious this was perhaps the only occurrence of this property on a selector, in larger scenarios it becomes harder to enforce this.

#8 in reply to: ↑ 6 @netweb
4 years ago

Replying to hnla:

Preserving the human readable formatting keeping the hex codes lined up.

Yes, for the most part big blocks of CSS like your example I've added "ignore" rules so that it remains human readable :)

I presume we can change the css comments to forward slash (These comments are after all not comments we want compiled to a true stylesheet) as per my example in the sheets where I cancel the check on vendor prefix for a mixins block.

A great example, thanks, there is a lot of CSS to read through ;)

I just tested this and using the double slash comments for the // stylelint-disable some-rule-here ignore rules works perfectly so that these are not compiled to the resulting CSS :)

#9 in reply to: ↑ 7 @netweb
4 years ago

Replying to hnla:

Yes, the coding standards state: "Use shorthand (except when overriding styles) for background, border, font, list-style, margin, and padding values as much as possible. (For a shorthand reference, see CSS Shorthand."

And how does this automated test determine when styles are attempting to override something that perhaps is located somewhere in the Cascade?

This is one of the great things about PostCSS (stylelint is a PostCSS plugin), it loads up the entire file CSS, SCSS, Less, whatever into an abstract syntax tree (AST) to iterate over all the properties, values, declarations at once. PostCSS plugins such as stylelint can then analyse where each declaration is in the AST tree to determine if it is related to another declaration, property, or value elsewhere in the AST tree.

Or for a more technical explanation that is far better than I could ever explain myself:

http://davidtheclark.com/its-time-for-everyone-to-learn-about-postcss/

"The tool itself is a Node.js module that parses CSS into an abstract syntax tree (AST); passes that AST through any number of “plugin” functions; and then converts that AST back into a string, which you can output to a file. Each function the AST passes through may or may not transform it; sourcemaps will be generated to keep track of any changes."

"The AST provides a straightforward API that developers can use to write plugins. For example, you can cycle through each rule set in a file with css.eachRule(), or each declaration in a rule with rule.eachDecl(). You can get the selector of a rule with rule.selector, or the name of an at-rule with atRule.name. From these few examples you can see that the PostCSS API makes it pretty easy to work with CSS source code (much easier and more accurately than if you were to rely on regular expressions, like a chump)."



Replying to hnla:

The reference to 'shorthand' actually covers both:

property: 0 0 0;

and

property: 0 0 0 0;

Programatically it is next to impossible to tell why one is used over another and neither is actually incorrect ( this is very similar to an issue that always used to exist in the jigsaw validator).

Aspects like this of CSS coding are tricky and where we always preferred that people would write 0 0 0 especially if obvious this was perhaps the only occurrence of this property on a selector, in larger scenarios it becomes harder to enforce this.

Correct, neither are technically incorrect, but WordPress has chosen the property: 0 0 0 type format, why this was chosen over the other is one for the history books and that I do not know. I'd has at a guess like many of the other CSS coding standards it has evolved over many years to be the "more common" style format used throughout the codebase so it was then standardised as the preferred format.

This codex article goes into the WordPress CSS shorthand coding standards a little further in regard to CSS shorthand https://codex.wordpress.org/CSS_Shorthand

#10 follow-up: @hnla
4 years ago

but WordPress has chosen the property: 0 0 0 type format,

Hmm it's not really a choice WP has made, if you need to state top right and bottom and if left is to have the same value as right then it's poor coding to state the lefts unit when CSS allows for inheritance of values when not explicitly stated, so this isn't a choice but the way one should write values; WP in this sense is simply following accepted practices - a good thing!

So I can see postcss is quite smart, but it is still only a automated tool, historically CSS has never been a language that can be dealt with this way, as smart or smarter that postcss might be it is still not going to be aware of the full cascade a site may be handling thus it can't know if I'm having to declare all units for a specific reason.

Lets ignore this though for the moment and focus on your @todo items earlier as then this can be committed and linting to these new rules can occur.

#11 in reply to: ↑ 10 @netweb
4 years ago

Replying to hnla:

Lets ignore this though for the moment and focus on your @todo items earlier as then this can be committed and linting to these new rules can occur.

Cool, much of the todo bits are in that DM Slack conversation we had a while ago, I'll dig into that later today go from there :)

#12 @netweb
4 years ago

  • Summary changed from Use stylelint to lint SCSS & CSS replacing Ruby gem `scss_lint` to Use stylelint to lint SCSS & CSS replacing Ruby Gem `scss_lint`

#13 @netweb
4 years ago

  • Description modified (diff)
Note: See TracTickets for help on using tickets.