Opened 4 months ago
Closed 3 months ago
#53866 closed enhancement (fixed)
Measurement in 'px' is unnecessary
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (32)
#2
@
4 months 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!
#5
follow-up:
↓ 13
@
4 months 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?
#7
@
4 months 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?
#9
@
4 months ago
@pbiron I have uploaded the updated patch.
Please check it and let me know for any changes.
Thanks
#12
follow-up:
↓ 14
@
4 months 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
@
4 months ago
#14
in reply to:
↑ 12
@
4 months 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.
4 months ago
#16
@
4 months ago
see this discuss in Slack re: automated tooling to flag and/or correct uses of 0px
.
#17
@
4 months 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:
↓ 20
@
4 months 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
@
4 months ago
@mukesh27 Thank you for bringing this to our attention. I'll upload the updated patch soon.
#20
in reply to:
↑ 18
@
4 months ago
Replying to mukesh27:
@ankitmaru Patch looks good to me.
We should also change/remove
padding: 0 0;
topadding: 0;
andmargin: 0 0;
tomargin: 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
#21
follow-up:
↓ 22
@
4 months 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 install
running 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
#22
in reply to:
↑ 21
@
4 months 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 runningnpm install
running 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-valuesThe above is for SCSS files, for CSS run:
./node_modules/.bin/stylelint "**/*.css" --config=.stylelintrc.js
This ticket was mentioned in PR #1604 on WordPress/wordpress-develop by ryelle.
4 months ago
Trac ticket: https://core.trac.wordpress.org/ticket/53866
#24
follow-up:
↓ 25
@
4 months 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.
#26
follow-up:
↓ 27
@
4 months 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
@
4 months 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/themesWith 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
) — exceptwp-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 /vendor
s 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
#29
@
3 months ago
- Keywords commit added
https://github.com/WordPress/wordpress-develop/pull/1604 looks good to me @ryelle
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?