-
Notifications
You must be signed in to change notification settings - Fork 204
feat(meter): s2 migration, full corner radius, help text #3968
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
feat(meter): s2 migration, full corner radius, help text #3968
Conversation
File metricsSummaryTotal size: 1.43 MB*
File change detailsmeter
progressbar
* An ASCII character in UTF-8 is 8 bits or 1 byte. |
🚀 Deployed on https://pr-3968--spectrum-css.netlify.app |
🦋 Changeset detectedLatest commit: 914a4f6 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 |
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 don't think this needs much more than small fixes! I made some callouts about documentation and a custom prop that I thought could use renaming. I also noticed that the meter spec has static white and static black examples/tokens included, but so far we're only supporting static white (somehow it seems like progress bar only needs static white, I guess that's why)? Should static black be added too?
📚 Branch previewPR #3968 has been deployed to Azure Blob Storage: https://spectrumcss.z13.web.core.windows.net/pr-3968/index.html. |
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 is really close! I just had a few small callouts in the comments.
Also, since we talked about adding side label for progress bar and discovered that it is in there but isn't well documented, how would you feel about updating the docs page and controls to include the side label variant?
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 had some suggestions for the changeset and would recommend enabling the side label control for progress bar and deleting the todo comment, but don't consider that 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.
Overall, this is looking pretty good! I think I found some missing tokens, specifically the meter thickness ones for each t-shirt size. And then I'd love your perspective on the border radius- is it supposed to be on both ends of the fill?
I did have a question about the animation for meter's (and progress bar's) animation. It looks like the CSS animation has that jitter that we solved over in SWC (adobe/spectrum-web-components#5535). Do you think this migration is the right place to fix that, too? Or do you think that's a separate bug fix PR? If we don't pull the fix into our repo for S2, would that mean when SWC takes in any new S2 CSS, we'd break the animation again?
Looks like there's some missing tokens: the meter-thickness-* t-shirt sizing tokens. Looking through the tokens/dist, the values for them are all slightly different than the current progress-bar-thicknes-* t-shirt sizing. Maybe we need some mods to pass to meter again to use those? There also may be some context I don't have!
@@ -161,6 +166,7 @@ | |||
margin-inline-start: var(--mod-spacing-progressbar-label-to-text, var(--spectrum-progressbar-spacing-label-to-text)); | |||
margin-block-end: 0; | |||
margin-block-start: 0; | |||
align-self: center; |
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.
What should I be expecting with the align-self: center
? I'm not sure I'm seeing any changes if I toggle this on and off in the browser in the side label story. Is it a backup to the align-self: flex-start
that's on the top label/default progress bar class? I have to be missing something...
Screen.Recording.2025-06-25.at.4.27.12.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.
If the side label on the left wraps to another line the percentage label is fixated at the top. That's why I added the align-self: center
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.
Screen.Recording.2025-06-25.at.6.10.54.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.
ah beautiful- thank you! I didn't have the main label wrapped!
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.
Do you think we should explicitly set any of the passthroughs for field label? I know the font sizes are already correct, but I think we got some requests to be more explicit about the cascade, so would that mean creating custom properties (something like --spectrum-progressbar-percentage-font-size: var(--spectrum-font-size-100);
, and then using mods for field label (--mod-field-label-font-size: var(--spectrum-progresbar-percentage-font-size);`)?
It feels like we did that in other components, just to be really sure that we're declaring the nested component styles properly. And we're already "redeclaring" the line-height property, even though it's also the same as field label's line-height. Just want to make sure we're doing all the components the same when it comes to embedded components!
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 moved the --spectrum-progressbar-font-size
token to the label and percentage selector. It was in the rootClass selector and it wasn't causing any effect there.
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.
Awesome. 👍
Do you think we should be passing that font size value to a mod/passthrough for the field label as well? I don't think there would ever be a use case where the field label and the percentage would be different fonts, but do we want to make sure all the fonts are the same size, weight, etc?
--mod-field-label-font-size: var(--spectrum-progressbar-font-size)
?
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.
Stylelint doesn't like the --mod
approach with the label passthrough. I'm going to keep the original way for now.
@marissahuysentruyt The reason why I didn't use the token |
@marissahuysentruyt Added the magical animation you created in the SWC PR 🤩 |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.
@@ -161,6 +166,7 @@ | |||
margin-inline-start: var(--mod-spacing-progressbar-label-to-text, var(--spectrum-progressbar-spacing-label-to-text)); | |||
margin-block-end: 0; | |||
margin-block-start: 0; | |||
align-self: center; |
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.
ah beautiful- thank you! I didn't have the main label wrapped!
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.
Awesome. 👍
Do you think we should be passing that font size value to a mod/passthrough for the field label as well? I don't think there would ever be a use case where the field label and the percentage would be different fonts, but do we want to make sure all the fonts are the same size, weight, etc?
--mod-field-label-font-size: var(--spectrum-progressbar-font-size)
?
@@ -15,15 +15,19 @@ | |||
.spectrum-Meter { | |||
--mod-progressbar-max-size: var(--mod-meter-max-width, var(--spectrum-meter-maximum-width)); | |||
--mod-progressbar-min-size: var(--mod-meter-min-width, var(--spectrum-meter-minimum-width)); | |||
--spectrum-meter-help-text-to-progress-bar: var(--spectrum-spacing-75); | |||
--mod-progressbar-thickness: var(--spectrum-meter-thickness-medium); |
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.
@aramos-adobe thanks for adding the meter tokens in! You're right that they're the same as progress bar. When I made that comment yesterday, I was looking through the large/mobile values 🤦♀️ , which are, in fact, different. But either way, I think it's a good practice for us to be explicit with the tokens like this (even if they're the same for the time being). Thanks!
Description
S2 Meter Migration
This migration updates the meter component and the progress bar component. Both needed the track to have a full corner radius to make the rounded look. The progress bar's HTML template now includes the help text component but it is only used in the meter.
New updates
--sizeM
and--sizeXL
Since the progress bar and meter are now using the same tokens for track thickness, font size, and spacing, the following mods have been redacted:
--mod-progressbar-thickness
--mod-progressbar-font-size
--mod-progressbar-spacing-top-to-text
New mods
--mod-meter-help-text-to-progress-bar
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
Regression testing
Validate:
Screenshots
To-do list