Skip to:
Content

BuddyPress.org

Opened 10 years ago

Closed 10 years ago

Last modified 8 years ago

#6351 closed enhancement (fixed)

Add Grunt task to llint SCSS files

Reported by: netweb's profile netweb Owned by: djpaul's profile djpaul
Milestone: 2.3 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch
Cc:

Description

We should add a task to identify any SCSS coding standard issues which will help in #6291 #6338

Standards will match WordPress Core CSS as closely as possible:
https://make.wordpress.org/core/handbook/coding-standards/css/#commenting

Attachments (4)

6351-initial.diff (4.4 KB) - added by netweb 10 years ago.
6351-initial.2.diff (4.8 KB) - added by netweb 10 years ago.
6351-03.diff (4.8 KB) - added by netweb 10 years ago.
6351-04.diff (4.8 KB) - added by netweb 10 years ago.

Download all attachments as: .zip

Change History (14)

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


10 years ago

@netweb
10 years ago

#2 @netweb
10 years ago

In 6351-initial.2.diff​:

  • Adds a new Grunt task grunt scsslint to lint .scss files in src/bp-templates/bp-legacy/css/
  • Requires npm install to update Node package dependancies
  • Adds the scsslint task to BP default src task to validate files before commit

This initial patch uses the default SCSS-Lint rules per:

This results in ~248 errors (For example see https://gist.github.com/ntwb/34868c3aac20af68480f )

I'll start updating the SCSS-Lint config now to match WordPress CSS Coding standards as much as possible

@netweb
10 years ago

#3 @netweb
10 years ago

Using 6351.03.diff gets us very close, the below is the result of running grunt scsslint for twentyfifteen.scss

$ grunt scsslint 
Running "scsslint:core" (scsslint) task
Running scss-lint on core
src/bp-templates/bp-legacy/css/twentyfifteen.scss:31 [W] NameFormat: Name of variable `RemFontValue` should be written in all lowercase letters with hyphens instead of underscores
src/bp-templates/bp-legacy/css/twentyfifteen.scss:33 [W] DuplicateProperty: Property `font-size` already defined on line 32
src/bp-templates/bp-legacy/css/twentyfifteen.scss:33 [W] NameFormat: Name of variable `RemFontValue` should be written in all lowercase letters with hyphens instead of underscores
src/bp-templates/bp-legacy/css/twentyfifteen.scss:169 [W] ColorVariable: Color literals like `#555` should only be used in variable declarations; they should be referred to via variable everywhere else.
src/bp-templates/bp-legacy/css/twentyfifteen.scss:170 [W] ColorVariable: Color literals like `#fff` should only be used in variable declarations; they should be referred to via variable everywhere else.
src/bp-templates/bp-legacy/css/twentyfifteen.scss:179 [W] ColorVariable: Color literals like `#eee` should only be used in variable declarations; they should be referred to via variable everywhere else.
src/bp-templates/bp-legacy/css/twentyfifteen.scss:180 [W] ColorVariable: Color literals like `#ddd` should only be used in variable declarations; they should be referred to via variable everywhere else.
src/bp-templates/bp-legacy/css/twentyfifteen.scss:209 [W] ColorVariable: Color literals like `#F2F2F2` should only be used in variable declarations; they should be referred to via variable everywhere else.
src/bp-templates/bp-legacy/css/twentyfifteen.scss:209 [W] HexNotation: Color `#F2F2F2` should be written as `#f2f2f2`
src/bp-templates/bp-legacy/css/twentyfifteen.scss:214 [W] ColorVariable: Color literals like `#fff` should only be used in variable declarations; they should be referred to via variable everywhere else.
src/bp-templates/bp-legacy/css/twentyfifteen.scss:241 [W] ColorVariable: Color literals like `#555` should only be used in variable declarations; they should be referred to via variable everywhere else.
src/bp-templates/bp-legacy/css/twentyfifteen.scss:242 [W] ColorVariable: Color literals like `#fff` should only be used in variable declarations; they should be referred to via variable everywhere else.
src/bp-templates/bp-legacy/css/twentyfifteen.scss:261 [W] ColorVariable: Color literals like `#eee` should only be used in variable declarations; they should be referred to via variable everywhere else.
src/bp-templates/bp-legacy/css/twentyfifteen.scss:265 [W] ColorVariable: Color literals like `#fff` should only be used in variable declarations; they should be referred to via variable everywhere else.
src/bp-templates/bp-legacy/css/twentyfifteen.scss:269 [W] ColorVariable: Color literals like `#ddd` should only be used in variable declarations; they should be referred to via variable everywhere else.
src/bp-templates/bp-legacy/css/twentyfifteen.scss:282 [W] SelectorDepth: Selector should have depth of applicability no greater than 5, but was 6
src/bp-templates/bp-legacy/css/twentyfifteen.scss:292 [W] ColorVariable: Color literals like `#ddd` should only be used in variable declarations; they should be referred to via variable everywhere else.
src/bp-templates/bp-legacy/css/twentyfifteen.scss:358 [W] ImportantRule: !important should not be used
src/bp-templates/bp-legacy/css/twentyfifteen.scss:462 [W] ColorVariable: Color literals like `#ccc` should only be used in variable declarations; they should be referred to via variable everywhere else.
src/bp-templates/bp-legacy/css/twentyfifteen.scss:484 [W] ColorVariable: Color literals like `#ccc` should only be used in variable declarations; they should be referred to via variable everywhere else.
src/bp-templates/bp-legacy/css/twentyfifteen.scss:505 [W] ImportantRule: !important should not be used
Warning: Task "scsslint:core" failed. Use --force to continue.
 
Aborted due to warnings.

Just a matter of tweaking the remaining rules or updating the code the reflect the SCSS-Lint rules.

#5 @hnla
10 years ago

If the result of the above 'grunt' thing is that it fails to compile a file then a few things ought to be thought about please:


DuplicateProperty: Property font-size

this is how mixins in respect of font-size work where fallback is required for IE where rems are not groked the pixel size is stated first for browsers then overridden with rem unit for browsers that understand it, or do you prefer we simply drop pixel support?

ImportantRule: !important should not be used

This is a dangerous assertion, I'm more than aware of why this shouldn't be used and have railed against it'suse til blue in the face, however it's largely become a problem due to inexperienced WP coders believing or mis-understanding it's use or used to try and bludgeon their rules into sticking - once this has been used or a ruleset has been needlessly over weighted there arises situations where the only option is to use it despite the horror in having to as noted in my comment where used - it's a nice rule to impose but frankly the horse has bolted, of course we could simply try to overspecify our selectors but then that might lead to the next test fail

SelectorDepth: Selector should have depth of applicability no greater than 5

Although I think this might have ramifications as above, the particular occurence as noted could probably take scrutiny and possible refining, not really sure at this moment what is really the issue as the compiled css looks ok.

ColorVariable: Color literals like... (should only be used in variabals)

While I was going to add a section to manage color declarations via variables, I think forcing this as an absolute condition isn't right, but hey ho.

HexNotation: Color #F2F2F2 should be written as #f2f2f2

This is simply a hindrance and irelevent, no one should really care that much on something like this, many tools used to color pick will automatically use uppercase and if providing copy and paste will paste in that format meaning one has to then backspace over and re-write to lowercase.

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


10 years ago

@netweb
10 years ago

#7 @netweb
10 years ago

  • Keywords has-patch added; needs-patch removed

#8 @DJPaul
10 years ago

Noting here that grunt-scss needs a Ruby module: gem update --system && gem install scss-lint which is not ideal.

I think it's the only option at the moment though; I've looked through grunt-sass -> node-sass -> libsass, and libsass doesn't have any linting options.

#9 @djpaul
10 years ago

  • Owner set to djpaul
  • Resolution set to fixed
  • Status changed from new to closed

In 9697:

Build: add SCSS linter (grunt-scss-lint) to Grunt build process.

As part of the companion stylesheet changes for BP 2.3 (#6291 #6338), we'll be adding SCSS files to src/bp-templates/bp-legacy/css/ to help modernise our CSS workflow. This change adds a SCSS linter to help us maintain our code quality.

An empty src/bp-templates/bp-legacy/css/twentyfifteen.scss file has been added because the linter hangs if you tell it to run in a folder that does not contain any SCSS files. This will be replaced shortly with the real companion stylesheet for twentyfifteen (#6291).

As grunt-scss-lint relies on a ruby gem, the Travis-CI config has been modified to install it before running tests.

Fixes #6351, props netweb.

#10 @DJPaul
8 years ago

  • Component changed from Tools - Build Process to Build/Test Tools
Note: See TracTickets for help on using tickets.