Stylelint: Another round of cleanup #43247
Conversation
tbradsha
added
[Status] Needs Review
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
github-actions
bot
added
[Block] Ad
[Block] AI Assistant
[Block] Contact Form
github-actions
bot
added
[mu wpcom Feature] Verbum Comments
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Debug Helper plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Vaultpress plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Boost plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Protect plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Inspect plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Wpcomsh plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Automattic For agencies client plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryThis PR did not change code coverage! That could be good or bad, depending on the situation. Everything covered before, and still is? Great! Nothing was covered before? Not so great. 🤷 |
There was a problem hiding this comment.
For the rest I wasn't sure, so I just switched to them to placeholders and created a class with the original name that extends the placeholder.
Looking at the docs suggests you might be able to do like this:
.foobar, %foobar {
// Some stuff
}
.baz {
@extends %foobar
// more stuff
}instead of having to do like
%foobar {
// Some stuff
}
.foobar {
@extends %foobar
}
.baz {
@extends %foobar
// more stuff
}As for the rest of the comments, most are ignorable if you disagree.
| font-weight: 600; | ||
| // stylelint-disable-next-line declaration-property-unit-allowed-list -- this should be changed to a unitless value: https://developer.wordpress.org/coding-standards/wordpress-coding-standards/css/#values |
There was a problem hiding this comment.
What's up with the excessive indent here?
| font-weight: 600; | |
| // stylelint-disable-next-line declaration-property-unit-allowed-list -- this should be changed to a unitless value: https://developer.wordpress.org/coding-standards/wordpress-coding-standards/css/#values | |
| font-weight: 600; | |
| // stylelint-disable-next-line declaration-property-unit-allowed-list -- this should be changed to a unitless value: https://developer.wordpress.org/coding-standards/wordpress-coding-standards/css/#values |
| @@ -15,6 +15,7 @@ | |||
|
|
|||
| #wpadminbar{ | |||
| background: $masterbar-background !important; | |||
| box-shadow: inset 0 -1px 0 $masterbar-background; // Calypso --color-masterbar-background | |||
There was a problem hiding this comment.
Maybe keep the comment from below?
| box-shadow: inset 0 -1px 0 $masterbar-background; // Calypso --color-masterbar-background | |
| // Nav Unification - Masterbar - border below the Masterbar | |
| box-shadow: inset 0 -1px 0 $masterbar-background; // Calypso --color-masterbar-background |
| @@ -345,12 +347,6 @@ | |||
| color:var(--wp-admin-theme-color) | |||
| } | |||
|
|
|||
| /** Add new site button **/ | |||
There was a problem hiding this comment.
I'm a bit iffy on this one. While the #adminmenu can be combined with the above, logically it seems lines 348–392 may belong to a "Add new site button" feature and we're losing indication of that with this change.
| button { | ||
| color: var(--jp-gray-0); | ||
| } |
There was a problem hiding this comment.
Mismatched indent.
| button { | |
| color: var(--jp-gray-0); | |
| } | |
| button { | |
| color: var(--jp-gray-0); | |
| } |
| /** | ||
| * These styles are copied directly from @wordpress/interface | ||
| * in order to remove our dependency on this experimental package. | ||
| * | ||
| * TODO: Replace these styles with our own styles. | ||
| * | ||
| * complementary-area-header | ||
| */ |
There was a problem hiding this comment.
Semantically this seems like it should be more like this
| /** | |
| * These styles are copied directly from @wordpress/interface | |
| * in order to remove our dependency on this experimental package. | |
| * | |
| * TODO: Replace these styles with our own styles. | |
| * | |
| * complementary-area-header | |
| */ | |
| /** | |
| * These styles are copied directly from @wordpress/interface | |
| * in order to remove our dependency on this experimental package. | |
| * | |
| * TODO: Replace these styles with our own styles. | |
| */ | |
| // complementary-area-header |
| @@ -132,10 +133,6 @@ body.presentation-wrapper-fullscreen-parent { | |||
| opacity: 0.8; | |||
| } | |||
|
|
|||
| .presentation-wrapper-fullscreen .nav-fullscreen-button { | |||
| @@ -7,7 +7,7 @@ | |||
| transition: all .1s ease-in-out; | |||
| } | |||
|
|
|||
| .jp-button { | |||
| %jp-button { | |||
There was a problem hiding this comment.
This will have the same problem as the boost one above.
| @@ -59,12 +59,6 @@ | |||
| }; | |||
| } | |||
|
|
|||
| .footer nav { | |||
| @@ -72,7 +72,7 @@ | |||
| cursor: pointer; | |||
| } | |||
|
|
|||
| .remove { | |||
| %remove { | |||
There was a problem hiding this comment.
This one probably has breakage too, like the boost thing below.
kraftbj
added
[Status] Needs Author Reply
This PR addresses the following rules:
property-no-unknowndeclaration-property-unit-allowed-listscss/comment-no-emptyno-duplicate-selectorsscss/at-extend-no-missing-placeholderselector-id-patternProposed changes:
property-no-unknownRule: https://stylelint.io/user-guide/rules/property-no-unknown/
Violations: 4
Three of the violations were due to CSS Modules
:exportusage inprojects/js-packages/components/components/layout/col/style.module.scss, and one was due to a validshadow-colorReact Native property. The rule was enabled with those exceptions.declaration-property-unit-allowed-listRule: https://stylelint.io/user-guide/rules/declaration-property-unit-allowed-list/
Violations: 75
WPCS wants unit-less or
pxline-heightvalues (source). Due to behavioral differences in inheritance between unit and unit-less values, this doesn't have an automatic fix. I changed these in CRM, VaultPress, andpackages/formsafter verifying no breakage, and the rest I comment-ignored, as testing each change is not worth the time and it's more important we prevent future violations. I also removed one line that browsers ignored due to a typo (45676de).scss/comment-no-emptyRule: https://stylelint.io/user-guide/rules/scss/comment-no-empty/
Violations: 75
Nearly all of these were multiple lines of
//comments, with a "blank" comment for spacing. I simplified them or swapped them out for/** ... */style comments. I also addedprojects/packages/jetpack-mu-wpcom/src/features/custom-css/custom-css/css/blank.cssto.stylelintignore(bb6c05d), since presumably it's as-is for a reason.no-duplicate-selectorsRule: https://stylelint.io/user-guide/rules/no-duplicate-selectors/
Violations: 141
This one has the most controversial changes. In a few cases there were duplicates in the same selector list or two selectors right near each other that were easily collapsed. In many cases it involved combining selectors across tens or hundreds of lines of code, so theoretically some of the changes could affect cascade order. Given that most instances had multiple levels of specificity or a quite specific class name, I'm not too worried in the end, and a click through of some of the plugins showed no issues.
scss/at-extend-no-missing-placeholderRule: https://github.com/stylelint-scss/stylelint-scss/blob/master/src/rules/at-extend-no-missing-placeholder/README.md
Violations: 13
Instead of
@extend .some-class, this rule wants@extend %some-placeholder. In CRM, the classes weren't used, so they were just switched out. For the rest I wasn't sure, so I just switched to them to placeholders and created a class with the original name that extends the placeholder. I verified no change to the compiled CSS for the Inspect plugin.selector-id-patternRule: https://stylelint.io/user-guide/rules/selector-id-pattern/
Violations: 408
This one I kept disabled. We don't have the luxury of ensuring our selectors conform to a specific pattern given that our code extends various repos and languages.
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions: