Skip to content

chore(stylelint): audit and remove unnecessary stylelint properties #3346

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Oct 30, 2024
Merged

chore(stylelint): audit and remove unnecessary stylelint properties #3346

merged 4 commits into from
Oct 30, 2024

Conversation

@cdransf
Copy link
Member

@cdransf cdransf commented Oct 29, 2024 •
edited
Loading

Description

  • Adjusts nesting violation in checkbox and removes stylelint disable comments.
  • Removes unnecessary disable from colorhandle, colorloupe, site and swatch.
  • Updates stylelint config to ignore forced-color-adjust: preserve-parent-color
  • Adjusts property order and nesting in switch to resolve stylelint violations

How and where has this been tested?

Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.

Validation steps

  1. Fetch the branch
  2. Run yarn linter and verify that there are no max depth, property order, unknown custom property, max precision, declaration property no unknown or forced color violations

Regression testing

Validate:

  1. The documentation pages for at least two other components are still loading, including:
  • The pages render correctly, are accessible, and are responsive.
  1. If components have been modified, VRTs have been run on this branch:
  • VRTs have been run and looked at.
  • Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

To-do list

  • I have read the contribution guidelines.
  • I have updated relevant storybook stories and templates.
  • I have tested these changes in Windows High Contrast mode.
  • If my change impacts other components, I have tested to make sure they don't break.
  • If my change impacts documentation, I have updated the documentation accordingly.
  • ✨ This pull request is ready to merge. ✨

@cdransf cdransf added size-2 S ~6-18hrs; not hard or time consuming, one or two work days to complete. skip_vrt Add to a PR to skip running VRT (but still pass the action) ready-for-review labels Oct 29, 2024
@cdransf cdransf self-assigned this Oct 29, 2024
@changeset-bot
Copy link

changeset-bot bot commented Oct 29, 2024 •
edited
Loading

⚠️ No Changeset found

Latest commit: 8765292

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@cdransf cdransf force-pushed the cdransf/stylelint-disable-audit branch from c96a038 to 2b2ed76 Compare October 29, 2024 17:09
@github-actions
Copy link
Contributor

github-actions bot commented Oct 29, 2024 •
edited
Loading

🚀 Deployed on https://pr-3346--spectrum-css.netlify.app

@github-actions
Copy link
Contributor

github-actions bot commented Oct 29, 2024 •
edited
Loading

File metrics

Summary

Total size: 4.30 MB*
Total change (Δ): 🟢 ⬇ 0.56 KB (-0.01%)

Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.

Package Size Δ
checkbox 24.20 KB 🟢 ⬇ 0.01 KB
dial 11.25 KB 🟢 ⬇ 0.01 KB
slider 32.24 KB 🟢 ⬇ 0.17 KB

Details

checkbox

Filename Head Compared to base
index-base.css 23.58 KB 🟢 ⬇ 0.01 KB (-0.05%)
index-theme.css 1.24 KB -
index-vars.css 24.20 KB 🟢 ⬇ 0.01 KB (-0.05%)
index.css 24.20 KB 🟢 ⬇ 0.01 KB (-0.05%)
themes/express.css 0.95 KB -
themes/spectrum.css 0.94 KB -

dial

Filename Head Compared to base
index-base.css 11.25 KB 🟢 ⬇ 0.01 KB (-0.06%)
index-vars.css 11.25 KB 🟢 ⬇ 0.01 KB (-0.06%)
index.css 11.25 KB 🟢 ⬇ 0.01 KB (-0.06%)

slider

Filename Head Compared to base
index-base.css 29.93 KB 🟢 ⬇ 0.17 KB (-0.55%)
index-theme.css 2.92 KB -
index-vars.css 32.24 KB 🟢 ⬇ 0.17 KB (-0.51%)
index.css 32.24 KB 🟢 ⬇ 0.17 KB (-0.51%)
themes/express.css 1.78 KB -
themes/spectrum.css 1.75 KB -
* Size determined by adding together the size of the main file for all packages in the library.
* Results are not gzipped or minified.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

@cdransf cdransf added run_vrt For use on PRs looking to kick off VRT and removed skip_vrt Add to a PR to skip running VRT (but still pass the action) labels Oct 29, 2024
castastrophe
castastrophe approved these changes Oct 30, 2024
View reviewed changes
Copy link
Contributor

@castastrophe castastrophe left a comment

Choose a reason for hiding this comment

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

Approved however some of the switch updates might be challenging to merge in with the foundations work. I think it should be fine though. I'll take the rebase carefully.

marissahuysentruyt
marissahuysentruyt reviewed Oct 30, 2024
View reviewed changes
Copy link
Collaborator

@marissahuysentruyt marissahuysentruyt left a comment

Choose a reason for hiding this comment

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

I sent you a Slack as well, but when I run yarn linter I see 2 errors in my terminal for switch and swatch regarding max-nesting-depth.

Screenshot 2024-10-30 at 10 19 25 AM Screenshot 2024-10-30 at 10 13 13 AM

@cdransf cdransf force-pushed the cdransf/stylelint-disable-audit branch 2 times, most recently from 4196d36 to 0de5503 Compare October 30, 2024 15:53
@cdransf cdransf added skip_vrt Add to a PR to skip running VRT (but still pass the action) and removed run_vrt For use on PRs looking to kick off VRT labels Oct 30, 2024
marissahuysentruyt
marissahuysentruyt approved these changes Oct 30, 2024
View reviewed changes
Copy link
Collaborator

@marissahuysentruyt marissahuysentruyt left a comment

Choose a reason for hiding this comment

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

Looks great! I don't think I see any of the errors you mentioned in the PR description 🏆

This may be in other work or cards, maybe we don't want addressing them, but I wanted to just list a few of the other stylelint errors I see in my terminal:

plugin/no-low-performance-animation-properties (various components)
selector-class-pattern (i.e. typography, stepper)
spectrum-tool/no-unknown-custom-properties (i.e. infield button, radio)
spectrum-tools/no-unused-custom-properties (i.e. slider, table)
order/order (i.e. tooltip)

@cdransf cdransf force-pushed the cdransf/stylelint-disable-audit branch from 0de5503 to e1843d8 Compare October 30, 2024 16:30
@cdransf cdransf force-pushed the cdransf/stylelint-disable-audit branch from e1843d8 to 8765292 Compare October 30, 2024 16:30
@cdransf cdransf merged commit 469f731 into main Oct 30, 2024
12 checks passed
@cdransf cdransf deleted the cdransf/stylelint-disable-audit branch October 30, 2024 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@marissahuysentruyt marissahuysentruyt marissahuysentruyt approved these changes

@jawinn jawinn Awaiting requested review from jawinn

@5t3ph 5t3ph Awaiting requested review from 5t3ph

+1 more reviewer

@castastrophe castastrophe castastrophe approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@cdransf cdransf

Labels

ready-for-review size-2 S ~6-18hrs; not hard or time consuming, one or two work days to complete. skip_vrt Add to a PR to skip running VRT (but still pass the action)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@cdransf @castastrophe @marissahuysentruyt