Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update stylelint & stylelint-config-wagtail packages, adopt a subset of new rules #11151

Merged

Conversation

lb-
Copy link
Member

@lb- lb- commented Oct 28, 2023

Overview

Closes #10719

This PR does not aim to align 100% with the new stylelint-config-wagtail updates but will ensure we do not get a huge amount of warnings when running npm install.

Instead, the goal is to adopt any auto 'fixing' of our styles files and a subset of easy to adopt rules without this PR being a huge item to review.

This also means that new styles added can align with the rules as they are in place and we avoid adding more complex styles than we already have (i.e. in the area of selector-max-combinators or selector-max-specificity.

Implementation notes

  • Ensure that some rules are always ignored in overrides (aka vendor) styles. This avoids us having to add ignore comments all over these styles.
  • Formatting & ordering (auto applied), this includes the padding/inset shorthand changes
  • Adopt a small set of no-union-classes changes as an example of changes to come in future PRs
  • Ignore some areas where we are selecting against data* attributes that will not be practical to change
  • Move some no-important ignore rules to specific lines
  • Ignore max-combinators in modeladmin styles (legacy)
  • Remove error messages forced-color-adjust setting to none

Testing notes

There is only one functional change: Messages in high contrast mode (with and without dark mode).

Future work

There are potentially 100s of additional changes to make to fully align with the stylelint-config-wagtail base styles.

Once merged, I will look at raising some issues that help us get incremental steps towards alignment, these will be potentially good first issues (for ones that are just renames of classes for example).

@squash-labs
Copy link

squash-labs bot commented Oct 28, 2023

Manage this branch in Squash

Test this branch here: https://lb-feature10719-update-styleli-1c89f.squash.io

@lb- lb- force-pushed the feature/10719-update-stylelint-packages branch 2 times, most recently from 7ed7473 to 29f7d27 Compare November 5, 2023 04:14
@lb- lb- force-pushed the feature/10719-update-stylelint-packages branch from 29f7d27 to 7c3b269 Compare December 3, 2023 06:35
@lb-
Copy link
Member Author

lb- commented Dec 3, 2023

Updated to be less strict again about block ordering & removed/cleaned up a few ordering items. Also resolved a few conflicts.

Would be great to get a review on this one so it does not have to be kept up to date with ongoing work in Wagtail. I have done my best to keep this PR to an absolute minimum and it helps our Node install process from showing so many warnings.

See wagtail/stylelint-config-wagtail#37 & wagtail/stylelint-config-wagtail#39

@lb- lb- force-pushed the feature/10719-update-stylelint-packages branch from 7c3b269 to 365622f Compare December 3, 2023 07:42
@lb-
Copy link
Member Author

lb- commented Dec 12, 2023

Need to manually rebase.

@lb- lb- force-pushed the feature/10719-update-stylelint-packages branch from 365622f to 6cd22e6 Compare December 12, 2023 21:31
@thibaudcolas thibaudcolas force-pushed the feature/10719-update-stylelint-packages branch from 6cd22e6 to 1913885 Compare December 18, 2023 12:52
Copy link
Member

@thibaudcolas thibaudcolas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @lb-! I’d like to further discuss declaration-block-no-redundant-longhand-properties before we enforce it in Wagtail so I’ve taken those changes out for now, I hope that’s ok.

I’ve also moved the overrides to overrides/** to be directly inside the files – as there are fewer than what the config suggests.

Also worth noting this includes a Sass update which introduces deprecation warnings (one related to shorthand properties which I took out, one which I’ll address separately). When we update our dependencies via transitive updates like this, I think it’d be nice to keep the package.json up-to-date – so I’ve done that.

This doesn’t change which version is installed, just avoids a bit of confusion if our package.json says ^1.0.0 or similar but actually it’s resolving to 1.69.5.

.stylelintrc.js Outdated
'max-nesting-depth': 5,
'selector-attribute-name-disallowed-list': null,
'selector-class-pattern': null,
'selector-max-combinators': 5,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see only two of those rules currently raising issues, and only in two files from our overrides, so I think we’d be better off with in-file overrides here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call thank you.

- Ensure that some rules are always ignored in overrides (aka vendor) styles. This avoids us having to add ignore comments all over these styles.
- Resolves wagtail#10719
Where possible, adopt a subset of stylelint-config-wagtail changes.

- Formatting & ordering (auto applied), this includes the padding/inset shorthand changes
- Adopt a small set of no-union-classes changes as an example of changes to come in future PRs
- Ignore some areas where we are selecting against data* attributes that will not be practical to change
- Move some no-important ignore rules to specific lines
- Ignore max-combinators in modeladmin styles (legacy)
- Remove error messages forced-color-adjust setting to none
@thibaudcolas thibaudcolas force-pushed the feature/10719-update-stylelint-packages branch from eb7765c to 5c4535d Compare December 18, 2023 13:28
@thibaudcolas thibaudcolas merged commit ffc6ac8 into wagtail:main Dec 18, 2023
11 of 14 checks passed
@thibaudcolas
Copy link
Member

I’ve opened wagtail/stylelint-config-wagtail#41 and wagtail/stylelint-config-wagtail#42 as possible follow-ups.

@lb- lb- deleted the feature/10719-update-stylelint-packages branch December 19, 2023 20:29
@lb-
Copy link
Member Author

lb- commented Dec 19, 2023

Thanks @thibaudcolas - I agree with the shorthand properties thing. From the outside it seems like a helpful feature but it does make changing code harder. Thanks for reviewing and resolving those items.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accessibility dependencies Pull requests that update a dependency file type:Cleanup/Optimisation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Stylelint to v15* & update stylelint-config-wagtail
2 participants