Make WordPress Core

Opened 10 years ago

Last modified 7 years ago

#26905 assigned defect (bug)

Follow CSS coding standards for SASS/SCSS files

Reported by: morganestes's profile morganestes Owned by: netweb's profile netweb
Milestone: Future Release Priority: normal
Severity: normal Version: 3.8
Component: Build/Test Tools Keywords:
Focuses: ui Cc:

Description

My first glance at _mixins.scss caught my eye with formatting that doesn't match the existing CSS Coding Standards. If we start the files out with the right formatting now while they're relatively small and few in number, they'll be easier to maintain as we start breaking up the CSS files into smaller module SASS files.

This might seem nitpicky now, but catching it early will help out in the long run.

Attachments (4)

26905-mixins.patch (2.5 KB) - added by morganestes 10 years ago.
wp-admin/css/colors/_mixins.scss
26905-admin.patch (10.3 KB) - added by morganestes 10 years ago.
Mostly style, but some minor refactoring as well.
26905-variables.patch (2.3 KB) - added by morganestes 10 years ago.
26905.diff (4.6 KB) - added by netweb 9 years ago.

Download all attachments as: .zip

Change History (17)

@morganestes
10 years ago

wp-admin/css/colors/_mixins.scss

#1 @morganestes
10 years ago

I also realize we don't change files just to fix formatting, but I thought since we're starting out with these it'd be a good plan to start out with them matching the coding standard.

@morganestes
10 years ago

Mostly style, but some minor refactoring as well.

#2 @morganestes
10 years ago

  • Component changed from Formatting to General

#3 @morganestes
10 years ago

  • Keywords has-patch 2nd-opinion added

I'm new to SASS/SCSS, but having used LESS in several other projects I understand the concepts. I'm a bit torn on what to think about spacing in function calls. I'm used to doing it in PHP so it looks normal to see something like lighten( $link, 10% ), but when it gets to desaturate( lighten( $menu-background, 7% ), 7% ) it starts looking a bit weird to me next to other CSS rules.

The current CSS Coding Standards say no to padded spaces within things like rgb() and url(); I've never thought of them as function calls before, but it makes sense to follow that style for now with SASS functions that have the same syntax.

I think we may need to have a new standard for the language, or include SCSS syntax into the current CSS coding standard so that all new files follow a pattern that new contributors can easily read and understand. A lot of devs are going to learn how to write code from reading what's in core, so this is a chance to influence them for good.

#4 @SergeyBiryukov
10 years ago

  • Component changed from General to Administration
  • Version changed from trunk to 3.8

#5 @SergeyBiryukov
10 years ago

  • Focuses ui added

@netweb
9 years ago

#6 @netweb
9 years ago

  • Component changed from Administration to Build/Test Tools
  • Keywords has-patch 2nd-opinion removed

Rather than patching our files, we should add a SCSS Lint Grunt task

In 26905.diff:

  • Adds a new Grunt task grunt scsslint to lint .scss files in src/bp-templates/bp-legacy/css/
  • Adds the scsslint task to WP's grunt precommit task to validate files before commit

https://www.npmjs.com/package/grunt-scss-lint
https://github.com/brigade/scss-lint

The rules in the patch in .scss-lint.yml get us pretty close to matching WordPress Core CSS standards, a few tweaks to either the .scss files in core or to these rules should get us where we'd like to be.

https://make.wordpress.org/core/handbook/coding-standards/css/

#7 @netweb
9 years ago

  • Milestone changed from Awaiting Review to Future Release
  • Owner set to netweb
  • Status changed from new to assigned

Related: #29792 Grunt: Add a precommit task to check for CSS syntax errors

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

#8 @netweb
8 years ago

  • Milestone changed from Future Release to 4.4

Moving to 4.4 as this will be included as part of the linting with Stylelint in #29792

Note: The downside of the previous proposal I added in comment 6 to use "SCSS Lint" is that a prerequisite is to also install the Ruby gem, which is not ideal for a myriad of reasons ;)

#9 @netweb
8 years ago

See #29792 and patch 29792.3.diff that includes Stylelint rules supporting SCSS files, updated SCSS changes patch coming shortly.

#10 @netweb
8 years ago

Punting this per #29792 as more still needs to be done here.

#11 @netweb
8 years ago

  • Milestone changed from 4.4 to Future Release

#12 follow-up: @fatmedia
7 years ago

FWIW, scss-lint seems to be going away: https://github.com/brigade/scss-lint#notice-consider-other-tools-before-adopting-scss-lint

I've been using this config in my own projects for a while and ran it through an automated tool to convert it to sass-lint. The result isn't perfect, but it's close enough for my purposes.

You all may prefer to use a different linter, but I figured I'd leave a heads-up either way.

#13 in reply to: ↑ 12 @netweb
7 years ago

Replying to fatmedia:

FWIW, scss-lint seems to be going away: https://github.com/brigade/scss-lint#notice-consider-other-tools-before-adopting-scss-lint

I've been using this config in my own projects for a while and ran it through an automated tool to convert it to sass-lint. The result isn't perfect, but it's close enough for my purposes.

You all may prefer to use a different linter, but I figured I'd leave a heads-up either way.

Thanks @fatmedia, there is a stylelint config for WordPress, SCSS support is not complete, primarily because WordPress hasn't defined any official SCSS Coding Standards, for the most part it follows the CSS standards.

Anyway here it is and later today I'll be releasing an update of the config https://www.npmjs.com/package/stylelint-config-wordpress

Note: See TracTickets for help on using tickets.