-
Notifications
You must be signed in to change notification settings - Fork 202
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
Conversation
🦋 Changeset detectedLatest commit: 4ba08ae The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
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 |
1798adf
to
6c4d57e
Compare
🚀 Deployed on https://pr-3908--spectrum-css.netlify.app |
File metricsSummaryTotal size: 1.42 MB*
File change detailsmeter
progressbar
* An ASCII character in UTF-8 is 8 bits or 1 byte. |
708606c
to
4d88fe5
Compare
There was a problem hiding this 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.
aefee32
to
dd34a6b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e143e64
to
2c0d669
Compare
4c0e530
to
209d3c5
Compare
There was a problem hiding this 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
There was a problem hiding this 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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…ate to support proper width token and sizes for medium and large variants
…e absence of customWidth arg
…r-size-default with newly defined inline-size token
78a1279
to
4ba08ae
Compare
Description
CSS-1039
This addresses a bug where the meter and progress bar components would not respond to changes in the
medium
andlarge
size controls.inline-size
from meterinline-size
token for meter to appropriate token in progress bar and sets itHow and where has this been tested?
Verified locally in Storybook.
Validation steps
@marissahuysentruyt
.progressbar
the--spectrum-progressbar-inline-size
evaluates to192px
whenMedium
is selected.--spectrum-progressbar-inline-size
evaluates to240px
whenLarge
is selected..progressbar
the--spectrum-progressbar-inline-size
evaluates to192px
whenMedium
is selected.--spectrum-progressbar-inline-size
evaluates to240px
whenLarge
is selected.Regression testing
Validate:
Screenshots
To-do list