Skip to content

fix(progress-bar + meter): Adjust progressbar styles, story and template to support proper width token and sizes for medium and large variants #3908

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

Merged
merged 5 commits into from
Jun 10, 2025

Conversation

cdransf
Copy link
Member

@cdransf cdransf commented Jun 5, 2025

Description

CSS-1039

This addresses a bug where the meter and progress bar components would not respond to changes in the medium and large size controls.

  • Removes custom width control and dependent code
  • Removes mod setting inline-size from meter
  • Aliases inline-size token for meter to appropriate token in progress bar and sets it

How and where has this been tested?

Verified locally in Storybook.

Validation steps

@marissahuysentruyt

  1. Open the preview URL.
  2. Navigate to the meter component and inspect it.
  3. Verify that for .progressbar the --spectrum-progressbar-inline-size evaluates to 192px when Medium is selected.
  4. Verify that --spectrum-progressbar-inline-size evaluates to 240px when Large is selected.
  5. Navigate to the progress bar component and inspect it.
  6. Verify that for .progressbar the --spectrum-progressbar-inline-size evaluates to 192px when Medium is selected.
  7. Verify that --spectrum-progressbar-inline-size evaluates to 240px when Large is selected.

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.

Screenshots

Screenshot 2025-06-06 at 11 10 24 AM Screenshot 2025-06-06 at 11 10 14 AM Screenshot 2025-06-06 at 11 09 44 AM Screenshot 2025-06-06 at 11 09 36 AM

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 self-assigned this Jun 5, 2025
@cdransf cdransf added size-3 M ~18-30hrs; moderate effort or complexity, several work days needed. wip This is a work in progress, don't judge. do not merge A flag for a branch indicating it should not be merged. skip_vrt Add to a PR to skip running VRT (but still pass the action) labels Jun 5, 2025
Copy link

changeset-bot bot commented Jun 5, 2025

🦋 Changeset detected

Latest commit: 4ba08ae

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@spectrum-css/progressbar Minor
@spectrum-css/meter Minor
@spectrum-css/bundle Patch
@spectrum-css/preview Patch

Not sure what this means? Click here to learn what changesets are.

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

@cdransf cdransf changed the base branch from main to spectrum-two June 5, 2025 23:20
@cdransf cdransf force-pushed the cdransf/progress-bar--meter-default-width branch from 1798adf to 6c4d57e Compare June 5, 2025 23:21
Copy link
Contributor

github-actions bot commented Jun 5, 2025

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

Copy link
Contributor

github-actions bot commented Jun 5, 2025

File metrics

Summary

Total size: 1.42 MB*
No change in file sizes

Package Size Minified Gzipped
meter 1.57 KB 1.51 KB 0.61 KB
progressbar 8.62 KB 8.18 KB 1.63 KB

File change details

meter

Filename Head Minified Gzipped Compared to base
index.css 1.57 KB 1.51 KB 0.61 KB 🟢 ⬇ 0.20 KB
metadata.json 1.02 KB - - 🟢 ⬇ 0.16 KB

progressbar

Filename Head Minified Gzipped Compared to base
index.css 8.62 KB 8.18 KB 1.63 KB 🟢 ⬇ 0.37 KB
metadata.json 4.65 KB - - 🟢 ⬇ 0.09 KB
* Size is the sum of all main files for packages in the library.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

@cdransf cdransf force-pushed the cdransf/progress-bar--meter-default-width branch 2 times, most recently from 708606c to 4d88fe5 Compare June 6, 2025 16:43
@cdransf cdransf added ready-for-review and removed wip This is a work in progress, don't judge. do not merge A flag for a branch indicating it should not be merged. labels Jun 6, 2025
Copy link
Collaborator

@rise-erpelding rise-erpelding left a comment

Choose a reason for hiding this comment

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

Thanks for working out a fix despite all the confusion and uncertainty about these components!

Requesting changes partly because of my own ridiculousness saying we didn't need the XL progress bar when it looks like it does still exist, but also to address that customWidth arg and story I mentioned in the comments.

I do also think that asking design specifically about whether we should use that --meter-width token for setting default width in S2 is a good idea. I'm assuming that the guidance we saw about default width on both progress-bar and meter would be the same for S2? But I don't see a size-2400 token, and I don't believe there's a default width token specified in either S1 or S2 token specs.

@cdransf cdransf force-pushed the cdransf/progress-bar--meter-default-width branch from aefee32 to dd34a6b Compare June 9, 2025 15:40
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.

This is looking pretty good! Just a few small changes from me 😊

I couldn't comment inline, but now that we added XL back in, would you remove the size2800 prop?
Screenshot 2025-06-09 at 12 57 09 PM

@cdransf cdransf force-pushed the cdransf/progress-bar--meter-default-width branch 4 times, most recently from e143e64 to 2c0d669 Compare June 9, 2025 18:19
@cdransf cdransf force-pushed the cdransf/progress-bar--meter-default-width branch 2 times, most recently from 4c0e530 to 209d3c5 Compare June 10, 2025 18:56
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 can't quite comment in the right spot, but starting at line 148, there are some old references to --spectrum-progressbar-size-default that are used to define the side labels, and the animation keyframes for the indeterminate progress bar. Can we update those to the new inline-size prop you made?

Current

Screen.Recording.2025-06-10.at.3.37.30.PM.mov

With the updated custom property references

Screen.Recording.2025-06-10.at.3.37.44.PM.mov

Copy link
Collaborator

@rise-erpelding rise-erpelding left a comment

Choose a reason for hiding this comment

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

This looks good! I left a couple of really small suggestions that you might add to the changeset, but would consider those non-blocking!

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.

nice

@cdransf cdransf force-pushed the cdransf/progress-bar--meter-default-width branch from 78a1279 to 4ba08ae Compare June 10, 2025 21:39
@cdransf cdransf merged commit 7971c77 into spectrum-two Jun 10, 2025
12 checks passed
@cdransf cdransf deleted the cdransf/progress-bar--meter-default-width branch June 10, 2025 21:49
@github-actions github-actions bot mentioned this pull request Jun 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-review size-3 M ~18-30hrs; moderate effort or complexity, several work days needed. skip_vrt Add to a PR to skip running VRT (but still pass the action)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants