Measurement in 'px' is unnecessary

[ankitmaru]

Measurement in 'px' is unnecessary - Admin CSS files

[audrasjb]

Hello and thank you for the ticket/patch @ankitmaru !

When a value is equal to zero, it doesn't need any unit of measure (zero pixel = zero em = zero % = zero rem = zero whatever) 🙂

Do you have any reason why the unit of measure should be added to values when they are equal to zero?

[audrasjb]

oh sorry I completely misread your ticket/patch. Going to grab some coffee now.
the patch looks good to me, thanks!

[ankitmaru]

@audrasjb Hahaha, Enjoy your coffee.

[audrasjb]

Thanks 😊☕️

I found a few more occurrences in Bundled themes though. Maybe we can commit this and open a dedicated ticket for them?

[ankitmaru]

@audrasjb Yup, We can.

[pbiron]

@ankitmaru Looking at the patch, it looks like there was a merge conflict in src/wp-admin/css/customize-controls.css. Can you looking into that and produce another patch?

[ankitmaru]

@pbiron Yes sure, I'll upload new patch soon.

[ankitmaru]

Updated patch.

[ankitmaru]

@pbiron I have uploaded the updated patch.
Please check it and let me know for any changes.
Thanks

[pbiron]

LGTM

[ayeshrajans]

LGTM x 2

[Hareesh Pillai]

Is there a way we can setup a process to detect and correct such anomalies automatically?
During the build process perhaps?

[ankitmaru]

I think yes, I have created separate ticket for that -> #53874
Replying to audrasjb:

Thanks 😊☕️

I found a few more occurrences in Bundled themes though. Maybe we can commit this and open a dedicated ticket for them?

[ankitmaru]

Thanks @hareesh-pillai for showing your interest.
Yes, It's very easy, Just use IDE search feature to find issues like this and study some CSS standard documents.

Replying to Hareesh Pillai:

Is there a way we can setup a process to detect and correct such anomalies automatically?
During the build process perhaps?

[pbiron]

see this discuss in ​Slack re: automated tooling to flag and/or correct uses of 0px.

[netweb]

Via https://core.trac.wordpress.org/ticket/53874#comment:3

This looks good, this change is also defined in the WordPress Coding Standards

​https://github.com/WordPress/gutenberg/blob/trunk/packages/stylelint-config/index.js#L67

Via the stylelint rule ​https://stylelint.io/user-guide/rules/list/length-zero-no-unit/

... eventually we'll get stylelint linting core, and passing 🤞🏼

[mukesh27]

@ankitmaru Patch looks good to me.

We should also change/remove padding: 0 0; to padding: 0; and margin: 0 0; to margin: 0; to avoid future tickets.

Here is one instance - ​https://github.com/WordPress/WordPress/blob/master/wp-admin/css/nav-menus.css#L435

[ankitmaru]

@mukesh27 Thank you for bringing this to our attention. I'll upload the updated patch soon.

[netweb]

Replying to mukesh27:

@ankitmaru Patch looks good to me.

We should also change/remove padding: 0 0; to padding: 0; and margin: 0 0; to margin: 0; to avoid future tickets.

Here is one instance - ​https://github.com/WordPress/WordPress/blob/master/wp-admin/css/nav-menus.css#L435

Good catch @ankitmaru

Whilst this is documented in the CSS coding standards here:

​https://developer.wordpress.org/coding-standards/wordpress-coding-standards/css/#properties

"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 shorthand-property-no-redundant-values rule is not currently configured in the @wordpress/stylelint-config package.

​https://stylelint.io/user-guide/rules/list/shorthand-property-no-redundant-values/

It would be great to get the example updated to highlight the incorrect example with the above here:

​https://github.com/WordPress/wpcs-docs/blob/master/wordpress-coding-standards/css.md#properties

And to also have this stylelint shorthand-property-no-redundant-values added to the @wordpress/stylelint-config package

​https://github.com/WordPress/gutenberg/blob/trunk/packages/stylelint-config/index.js

[netweb]

Going to remove the commit keyword from here for now pending the updated patch.

To help with checking for all cases of the two stylelint rules, if you add the below code to a file named .stylelintrc.js to the root of the project, and after running npm installrunning this command will display all instances of the code these two rules affect:

'use strict';

module.exports = {
        rules: {
                'length-zero-no-unit': true,
                'shorthand-property-no-redundant-values': true,
        },
};

Then run:

./node_modules/.bin/stylelint "**/*.scss" --config=.stylelintrc.js

And you should see something like this:

src/wp-content/themes/twentynineteen/style-editor.scss
 794:26  ✖  Unexpected unit   length-zero-no-unit
 795:25  ✖  Unexpected unit   length-zero-no-unit

src/wp-content/themes/twentytwentyone/assets/sass/01-settings/global.scss
 230:32  ✖  Unexpected unit   length-zero-no-unit

src/wp-content/themes/twentytwentyone/assets/sass/06-components/comments.scss
 130:3  ✖  Unexpected longhand value '8px 0 9px 0' instead of '8px 0 9px'   shorthand-property-no-redundant-values

src/wp-content/themes/twentytwentyone/assets/sass/06-components/navigation.scss
 301:6  ✖  Unexpected longhand value '0 7px 10px 7px' instead of '0 7px 10px'   shorthand-property-no-redundant-values

src/wp-content/themes/twentytwentyone/assets/sass/05-blocks/navigation/_style.scss
 84:6  ✖  Unexpected longhand value '0 7px 10px 7px' instead of '0 7px 10px'   shorthand-property-no-redundant-values

The above is for SCSS files, for CSS run:

./node_modules/.bin/stylelint "**/*.css" --config=.stylelintrc.js

[ankitmaru]

Thanks for the info.

Replying to netweb:

Going to remove the commit keyword from here for now pending the updated patch.

To help with checking for all cases of the two stylelint rules, if you add the below code to a file named .stylelintrc.js to the root of the project, and after running npm installrunning this command will display all instances of the code these two rules affect:

'use strict';

module.exports = {
        rules: {
                'length-zero-no-unit': true,
                'shorthand-property-no-redundant-values': true,
        },
};

Then run:

./node_modules/.bin/stylelint "**/*.scss" --config=.stylelintrc.js

And you should see something like this:

src/wp-content/themes/twentynineteen/style-editor.scss
 794:26  ✖  Unexpected unit   length-zero-no-unit
 795:25  ✖  Unexpected unit   length-zero-no-unit

src/wp-content/themes/twentytwentyone/assets/sass/01-settings/global.scss
 230:32  ✖  Unexpected unit   length-zero-no-unit

src/wp-content/themes/twentytwentyone/assets/sass/06-components/comments.scss
 130:3  ✖  Unexpected longhand value '8px 0 9px 0' instead of '8px 0 9px'   shorthand-property-no-redundant-values

src/wp-content/themes/twentytwentyone/assets/sass/06-components/navigation.scss
 301:6  ✖  Unexpected longhand value '0 7px 10px 7px' instead of '0 7px 10px'   shorthand-property-no-redundant-values

src/wp-content/themes/twentytwentyone/assets/sass/05-blocks/navigation/_style.scss
 84:6  ✖  Unexpected longhand value '0 7px 10px 7px' instead of '0 7px 10px'   shorthand-property-no-redundant-values

The above is for SCSS files, for CSS run:

./node_modules/.bin/stylelint "**/*.css" --config=.stylelintrc.js

Trac ticket: https://core.trac.wordpress.org/ticket/53866

[ryelle]

These issues can be autofixed by stylelint, so I created ​a PR that does that 🙂 Spot-checking it everything seems OK.

I used the .stylelintrc.js in 21, plus a .stylelintignore file. That way, the command ignores 3rd-party files (from Gutenberg), and themes (handled in #53874).

## .stylelintignore
# Ignore files that come from Gutenberg.
src/wp-includes/css/dist
src/wp-includes/blocks

# Ignoring themes for now, handle in #53874.
src/wp-content/themes

With those two files in place, the command to fix is:

./node_modules/.bin/stylelint "src/**/*.{css,scss}" --config=.stylelintrc.js --fix

I also ran npm run build:dev, to rebuild all the built files. Testing again after running these, everything passes the stylelint check (same command, without --fix) — except wp-admin/css/themes-rtl.css. It looks like the RTL build process expands the shorthand properties.

IMO that's okay since it's a generated file, but if we introduce a stylelint config for real, **/*-rtl.css and **/*.min.css should get ignored.

[pbiron]

Replying to ryelle:

These issues can be autofixed by stylelint, so I created ​a PR that does that 🙂 Spot-checking it everything seems OK.

Thank you!

Quick question: are the vendor/tinymce/.../xxx-min.css files generated by the WP build process or do they "come to us" only as minified files?

[ryelle]

Looks like those files come to us like that, so it makes sense to ignore them. I'll remove them from my PR.

Should I ignore all vendor/* files, or just the tinymce files? vendor/crop/cropper.css & vendor/thickbox/thickbox.css are the other two altered files, but it looks like those aren't updated in the same way as TinyMCE.

[netweb]

Replying to ryelle:

These issues can be autofixed by stylelint, so I created ​a PR that does that 🙂 Spot-checking it everything seems OK.

✅ Looks good @ryelle, just remove the /vendor changes per @pbiron comment

I used the .stylelintrc.js in 21, plus a .stylelintignore file. That way, the command ignores 3rd-party files (from Gutenberg), and themes (handled in #53874).

## .stylelintignore
# Ignore files that come from Gutenberg.
src/wp-includes/css/dist
src/wp-includes/blocks

# Ignoring themes for now, handle in #53874.
src/wp-content/themes

With those two files in place, the command to fix is:

./node_modules/.bin/stylelint "src/**/*.{css,scss}" --config=.stylelintrc.js --fix

I also ran npm run build:dev, to rebuild all the built files. Testing again after running these, everything passes the stylelint check (same command, without --fix) — except wp-admin/css/themes-rtl.css. It looks like the RTL build process expands the shorthand properties.

IMO that's okay since it's a generated file, but if we introduce a stylelint config for real, **/*-rtl.css and **/*.min.css should get ignored.

I started working on a stylelint config and ignore patch to land in the /root of the project, I found a couple of bugs in the WordPress stylelint config, so I'll get those fixed and a patch added for this in #29792

That should handle the min & rtl ignores as expected too.

---

Replying to ryelle:

Looks like those files come to us like that, so it makes sense to ignore them. I'll remove them from my PR.

Should I ignore all vendor/* files, or just the tinymce files? vendor/crop/cropper.css & vendor/thickbox/thickbox.css are the other two altered files, but it looks like those aren't updated in the same way as TinyMCE.

Yes, ignore all the CSS from any /vendors paths:

In this case, ignore these 5 files:

• src/js/_enqueues/vendor/crop/cropper.css
• src/js/_enqueues/vendor/thickbox/thickbox.css
• src/js/_enqueues/vendor/tinymce/plugins/compat3x/css/dialog.css
• src/js/_enqueues/vendor/tinymce/skins/lightgray/content.inline.min.css
• src/js/_enqueues/vendor/tinymce/skins/lightgray/content.min.css
• src/js/_enqueues/vendor/tinymce/skins/lightgray/skin.min.css

[ryelle]

Rebased & updated the PR, dropping the changes to vendor/*.

[netweb]

​https://github.com/WordPress/wordpress-develop/pull/1604 looks good to me @ryelle

[ryelle]

In 51727:

Coding Standards: Apply coding standards on CSS.

Remove units when the value is zero. Combine redundant values in shorthand declarations.
This was generated with stylelint --fix and a custom config (see #53866).

Props ankitmaru, audrasjb, pbiron, ayeshrajans, hareesh-pillai, netweb, mukesh27.
Fixes #53866.