Skip to:
Content

BuddyPress.org

Opened 8 years ago

Closed 7 years ago

Last modified 3 years ago

#7028 closed enhancement (fixed)

Use stylelint to lint SCSS & CSS replacing Ruby Gem `scss_lint`

Reported by: netweb's profile netweb Owned by: netweb's profile netweb
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

Attachments (2)

7028.patch (289.0 KB) - added by netweb 8 years ago.
7028.1-tooling.patch (11.8 KB) - added by netweb 7 years ago.

Download all attachments as: .zip

Change History (34)

@netweb
8 years ago

#1 @netweb
8 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
8 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
8 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
8 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
8 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
8 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
8 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
8 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
8 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
8 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
8 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
8 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
8 years ago

  • Description modified (diff)

#14 @netweb
8 years ago

  • Description modified (diff)

This ticket was mentioned in Slack in #buddypress by netweb. View the logs.


8 years ago

#16 @imath
8 years ago

Thanks @netweb the ruby thing is really a pain. It would be great to have this in!

#17 @DJPaul
8 years ago

  • Component changed from Tools - Build Process to Build/Test Tools

#18 @DJPaul
8 years ago

  • Milestone changed from Future Release to 2.7
  • Type changed from defect (bug) to enhancement

Can someone update where we are with this? It's been maturing for a while and I'd like to get it into trunk soon, so we have a full dev cycle to catch regressions.

#19 @netweb
8 years ago

  • Owner set to netweb
  • Status changed from new to assigned

I'll refresh the patch this week :+1

This ticket was mentioned in Slack in #buddypress by djpaul. View the logs.


8 years ago

#21 @DJPaul
7 years ago

  • Milestone changed from 2.7 to Future Release

#22 @netweb
7 years ago

Updated patch 7028.1-tooling.patch, this is the "tooling" only part of the patch, adds stylelint and grunt stylelint tasks, removes scss-lint.

I'll update the CSS and SCSS patches tomorrow on the plane to WordCamp Sydney

Edit: There's ~6,100 lint errors or warnings to be resolved

Last edited 7 years ago by netweb (previous) (diff)

#23 @hnla
7 years ago

@netweb

Edit: There's ~6,100 lint errors or warnings to be resolved

Why are there so many isn't this predominately just a change you want because you reckon it's better all round to use this module i.e dropping need for Ruby and other benefits.

Not clear on why the config or lint rules have changed to prompt so many warnings (errors is a little strong as most linting that I've seen isn't critical error handling type stuff). Can we not really get the new lint config rules to closer parity and thus less changes with the existing lint rules?

#24 @hnla
7 years ago

@netweb
Hmm running stylelint and think we can relax some lint rules like font-weight notation to allow keywords as well as numeric and likely there are likely other little rules like this that aren't ultra critical, majority of errors on core sheets seem to be ruleset spacing i.e newline after so those sheets could be corrected quite quickly, done two smaller ones could probably just commit those in without tickets to ease the workload as they're updated?

#25 @netweb
7 years ago

I've already got 95% of all this patched, including all the newlines etc, most of it is also already in 7028.patch, I'm just refreshing the latest changes leisurely on the plane today. I'll upload a refreshed patch in a few hours.

And to reiterate, the configuration is 100% based on WordPress Coding Standards as mentioned in the original ticket and follow ups, all rules are based on those standards.

#26 @hnla
7 years ago

@netweb ok sorry was just trying to help mate, wasn't sure how much of a pain point it was going to be for you especially as you pointed out the number of lines needing dealing with ;)

As for the oft mentioned Coding Standards yes I do get that, & regardless of whether the standards as set out are right or wrong, I checked and indeed your standards do specify to use numeric values for font-weight not keywords so I'll stand down on that one :)

#27 @hnla
7 years ago

  • Milestone changed from Future Release to 2.9

@netweb Any chance we can get this in for 2.9 and get parity with bbPress and ease the woes of the Mac Brigade :)

This ticket was mentioned in Slack in #buddypress by hnla. View the logs.


7 years ago

#29 @netweb
7 years ago

In [11546]:

Build/Test Tools: Add stylelint for CSS & SCSS linting.

This changeset is the first pass at adding stylelint. A new Grunt task grunt stylelint is now available and this will lint CSS & SCSS files against WordPress' CSS Coding Standards. Follow up commits will update BuddyPress' CSS & SCSS files to adhere to these coding standards. Following the CSS & SCSS updates the Ruby scss-lint gem will be removed.

Props netweb.
See #7028.

#30 @netweb
7 years ago

In 11550:

Build/Test Tools: Update stylelint-config-wordpress to 11.0.0.

See #7028.

#31 @netweb
7 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 11587:

Build Tools: Use stylelint to lint CSS and SCSS files.

  • Switches to using https://stylelint.io/ for CSS and SCSS linting
  • Removes the Ruby SCSS lint tool configuration and Grunt tasks
  • One less dependency platform for which BuddyPress repo tool chain.

Props netweb, hnla, DJPaul.
Fixes #7028.

#32 @netweb
7 years ago

In 11679:

Build Tools: Remove extraneous grunt-scss-lint

This changeset is a follow up to [11587] where grunt-scss-lint should have been removed from package.json and scss-lint linting comments should have also been removed from SCSS files.

See #7028.

Note: See TracTickets for help on using tickets.