Stylelint: report & clean needless disables#50174
Conversation
|
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. |
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
| // styles that try to re-style native inputs. | ||
| margin: 0; | ||
| padding: 0 !important; // stylelint-disable-line declaration-no-important | ||
| border: 0 !important; // stylelint-disable-line declaration-no-important |
There was a problem hiding this comment.
While declaration-no-important isn't enabled in Jetpack, it might make sense to enable repo wide. But not in this PR. :-)
There was a problem hiding this comment.
1355 instances of !important in .css and .scss files across 208 files would need cleaning up. 😉
~500 are in projects/plugins/jetpack/scss/cleanslate.scss and projects/plugins/jetpack/scss/wordads-ccpa.scss.
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. 🤷 |
| @@ -1,6 +1,3 @@ | |||
| /* Additional styling to thickbox that displays modal */ | |||
There was a problem hiding this comment.
OTOH, this comment seems like it should possibly be kept. It seems to be commenting on the file, not the disable.
There was a problem hiding this comment.
I think it can go; it refers to adding override styles for Thickbox, which was also the reason for the lint-disable.
Now the modal is created fresh from scratch entirely.
Co-authored-by: Brad Jorsch <anomiex@users.noreply.github.com>
Proposed changes
reportNeedlessDisables: truefor Stylelint: it turns these stale comments into CI failures instead of letting them accumulate.Related product discussion/links
See an AI-produced summary for each rule.
Here is a rule-by-rule breakdown of why those disables were added originally, and why `reportNeedlessDisables` correctly flagged them as dead weight.The common thread matches Gutenberg #79788: most were workarounds for older Stylelint versions or older
@wordpress/stylelint-configrules that no longer fire on the current toolchain (Stylelint 17 +@wordpress/stylelint-config23.x).scales/radiiWhy added: A legacy WordPress Stylelint plugin (
scales/radii) requiredborder-radiusvalues to match the Gutenberg design scale ($radius-small,$radius-medium, etc.). Raw values like8px,12px, or15pxwere flagged.Where it showed up: number-slider, social previews (threads/tumblr/twitter), backup storage meter, wpcom-global-styles toggle.
Why not needed anymore: The
scales/*plugin rules were removed from@wordpress/stylelint-configduring the Stylelint 15→17 migration. Jetpack extendsscss-stylistic, which no longer enables them. The comments outlived the rule.scales/font-sizesWhy added: Same
scales/*family — enforced the WordPress admin typography scale. Social preview components intentionally use platform-specific sizes (e.g. Facebook’s 15px /0.9375rem) that don’t match Gutenberg tokens.Where it showed up: bluesky, facebook, mastodon, tumblr previews.
Why not needed anymore: Same as
scales/radii— the rule is gone from the active config. Those pixel-perfect sizes are still valid CSS; only the lint enforcement disappeared.scales/font-weightsWhy added: Enforced allowed font-weight values from the design scale.
font-weight: 900on the event-countdown block was outside what the rule accepted.Where it showed up:
event-countdown.scss.Why not needed anymore: Another retired
scales/*rule — no longer part of the config.no-invalid-position-declarationWhy added: A relatively new core Stylelint rule (added in v16.11) that flags declarations in invalid positions within a rule block. It false-positived heavily in SCSS mixins, where properties inside
@mediablocks get expanded into output Stylelint couldn’t reason about correctly. Comments explicitly said “Ok in a sass mixin.”Where it showed up: forms
_mixins.scss, my-jetpack product-interstitial_mixins.scss(10 instances).Why not needed anymore: Stylelint fixed mixin false positives (#9120). Declarations like
margin-bottominside a mixin’s@mediablock are now understood correctly.selector-max-idWhy added: Limits ID selectors (
#foo), which WordPress coding standards discourage.memberships.scsshad a file-level disable, likely from when it styled Thickbox modal IDs (#TB_*).Where it showed up:
memberships.scss.Why not needed anymore: The file was refactored to classes and
<dialog>— there are no ID selectors left. The blanket disable was leftover from old Thickbox integration.no-duplicate-selectorsWhy added: Flags when the same selector appears twice in a file. In the contact-form editor,
border-left: none !importantwas split into a separate nested rule as a workaround for production CSS attribute reordering (comment in the file explains this). That structural split created a duplicate-selector pattern Stylelint flagged.Where it showed up:
contact-form/editor.scss.Why not needed anymore: Stylelint 17 improved
no-duplicate-selectorshandling (includingignoreSelectorssupport and fewer false positives). The workaround structure no longer triggers the rule.function-url-quotesWhy added:
@wordpress/stylelint-configsetsfunction-url-quotes: 'never'. Disables were added for quotedurl()values in inline SVG data URLs (mask-image: url("data:image/svg+xml,...")), where quotes are often required for parsing.Where it showed up:
wpcom-global-styles/notice.scss.Why not needed anymore: Newer Stylelint / config versions handle quoted data URLs correctly, or the specific
mask-imageusage no longer violates the rule. Gutenberg removed the same disable in #79788 for identicalmask-imagepatterns.declaration-property-unit-allowed-listWhy added: WordPress config restricts units on certain properties (historically centered on
line-height: pxonly). Disables were sprinkled onfont-sizedeclarations usingpx,rem, or%in wpcom blocks — likely copied from an era when the rule was broader, or from a misunderstanding of which properties it targets.Where it showed up: event-countdown, global-styles-sidebar, recommended-tags-modal.
Why not needed anymore: The current
@wordpress/stylelint-configonly restrictsline-height, notfont-size. Thosefont-sizedisables were suppressing a rule that never applied to those declarations.property-no-unknown+selector-pseudo-element-no-unknownWhy added: Stylelint didn’t recognize View Transitions API syntax when premium-analytics added popover animations:
view-transition-name::view-transition-group(),::view-transition-new(),::view-transition-old()Where it showed up:
date-comparison-dropdown.scss,date-range-filter.scss.Why not needed anymore: Stylelint’s built-in known-property and pseudo-element lists were updated to include View Transitions. The same change landed in Gutenberg core around the same time.
declaration-no-importantWhy added: Stylelint discourages
!importantas a maintainability smell. The WC price filter slider uses!importantonpaddingandborderto reset native range input styles that themes override.Where it showed up:
filter-wc-price/style.scss.Why not needed anymore:
declaration-no-importantis not enabled in Jetpack’s effective config chain (@wordpress/stylelint-config/scss-stylistic+ Jetpack overrides). The inline disables were guarding a rule that isn’t active.selector-pseudo-class-no-unknownWhy added: Flags unrecognized pseudo-classes. CSS Modules’
:global()pseudo-class isn’t standard CSS, so Stylelint flagged:global(.visx-bar)and:global(.visx-tooltip):has(.tooltip).Where it showed up: premium-analytics chart-bar and chart-tooltip modules.
Why not needed anymore: Jetpack’s base config already whitelists it:
The per-file disables predated (or duplicated) that central config.
Summary table
scales/radii@wordpress/stylelint-configscales/font-sizesscales/font-weightsno-invalid-position-declaration@mixinselector-max-id#idselectorsno-duplicate-selectorsfunction-url-quotesmask-imagedeclaration-property-unit-allowed-listline-height, notfont-sizeproperty-no-unknownselector-pseudo-element-no-unknown::view-transition-*declaration-no-important!importantselector-pseudo-class-no-unknown:global()stylelint.config.base.mjsDoes this pull request change what data or activity we track or use?
Testing instructions
CI lint passing?