Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#53866 closed enhancement (fixed)

Measurement in 'px' is unnecessary

Reported by: ankitmaru's profile ankitmaru Owned by: ryelle's profile ryelle
Milestone: 5.9 Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch commit
Focuses: css, coding-standards Cc:

Description

Measurement in 'px' is unnecessary - Admin CSS files

Attachments (2)

measure_px_is_unnecessary_css_53866.patch (6.3 KB) - added by ankitmaru 3 years ago.
measure_px_is_unnecessary_css_53866_updated.patch (6.0 KB) - added by ankitmaru 3 years ago.
Updated patch.

Download all attachments as: .zip

Change History (32)

#1 @audrasjb
3 years ago

  • Keywords close reporter-feedback added

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?

#2 @audrasjb
3 years ago

  • Keywords close reporter-feedback removed
  • Milestone changed from Awaiting Review to 5.9

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

#3 @ankitmaru
3 years ago

  • Keywords close reporter-feedback added

@audrasjb Hahaha, Enjoy your coffee.

#4 @ankitmaru
3 years ago

  • Keywords reporter-feedback removed

#5 follow-up: @audrasjb
3 years ago

  • Keywords close removed

Thanks 😊☕️

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

Last edited 3 years ago by audrasjb (previous) (diff)

#6 @ankitmaru
3 years ago

@audrasjb Yup, We can.

#7 @pbiron
3 years ago

@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?

#8 @ankitmaru
3 years ago

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

#9 @ankitmaru
3 years ago

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

#10 @pbiron
3 years ago

LGTM

#11 @ayeshrajans
3 years ago

LGTM x 2

#12 follow-up: @Hareesh Pillai
3 years ago

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

#13 in reply to: ↑ 5 @ankitmaru
3 years ago

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?

Last edited 3 years ago by SergeyBiryukov (previous) (diff)

#14 in reply to: ↑ 12 @ankitmaru
3 years ago

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?

This ticket was mentioned in Slack in #core-coding-standards by pbiron. View the logs.


3 years ago

#16 @pbiron
3 years ago

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

#17 @netweb
3 years ago

  • Keywords commit added

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 🤞🏼

#18 follow-up: @mukesh27
3 years ago

@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

#19 @ankitmaru
3 years ago

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

#20 in reply to: ↑ 18 @netweb
3 years ago

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

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

#21 follow-up: @netweb
3 years ago

  • Keywords commit removed

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

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

#22 in reply to: ↑ 21 @ankitmaru
3 years ago

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

#24 follow-up: @ryelle
3 years ago

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.

Last edited 3 years ago by ryelle (previous) (diff)

#25 in reply to: ↑ 24 @pbiron
3 years ago

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?

#26 follow-up: @ryelle
3 years ago

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.

#27 in reply to: ↑ 26 @netweb
3 years ago

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

#28 @ryelle
3 years ago

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

#29 @netweb
3 years ago

  • Keywords commit added

#30 @ryelle
3 years ago

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

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.

Note: See TracTickets for help on using tickets.