feat(slider): S2 migration, new precision variant, embedded textfield #3945
Conversation
🦋 Changeset detectedLatest commit: 48ae945 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
File metricsSummaryTotal size: 1.43 MB* Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.
File change detailsslider
tokens
* An ASCII character in UTF-8 is 8 bits or 1 byte. |
|
🚀 Deployed on https://pr-3945--spectrum-css.netlify.app |
aramos-adobe
changed the title
aramos-adobe
added
size-5 L
|
Things left to do:
|
📚 Branch previewPR #3945 has been deployed to Azure Blob Storage: https://spectrumcss.z13.web.core.windows.net/pr-3945/index.html. |
There was a problem hiding this comment.
First pass done. I got pulled into some other things late in the day, but I didn't want these to sit for too long! I'll do another check of the CSS tomorrow!
There's a few stories I'd like to see on the docs page- the large track height, the precision handle (we could even steal the docs about the precision handle from the token specs), and then the editable with the textfield. Is there any other Figma documentation we can use to help beef up the information we have? (which is not much 😆) or from the contributions site?
There was a problem hiding this comment.
I didn't say it before, but WOW! This is so complex. Thanks for taking this on.
-
For the focus ring, I think we should error on the side of not having a
--mod-focus-indicator-gap. If it was me, the first step would be to create a slider-specific focus indicator gap custom variable like we have in other components, instead of using the globalfocus-indicator-gaptoken in our styles, and then have a mod for that slider focus ring gap instead. Basically, we'd end up with something like--spectrum-slider-focus-indicator-gap: var(--spectrum-focus-indicator-gap), and then at lines 432 or so, have--mod-slider-focus-indicator-gapand--spectrum-slider-focus-indicator-gap. -
Speaking of focus states, what are the expectations for us to represent those? Right now, we're showcasing both focus on the handle and focus on the textfield at the same time, or focus on both range handles, but is that accurate? Like would that happen, is that a scenario that is expected, to have "focus" on 2 elements at once?
Figma:
In our tests:
Tabbing:
Screen.Recording.2025-06-27.at.1.54.54.PM.mov
- For the "Spacing (top/bottom edge to label) section in Figma- I'm not convinced that we're using at least the top-to-text tokens accurately. If I understood things right, those tokens are being applied to
.spectrum-Slider-labelContainer, which adds extra spacing after each labels padding. With Figma as the reference, that's not really where that spacing is supposed to go. Would it be acceptable to remove those? (like line 478?) If we can't or shouldn't remove them, can you update those to use the correct tokens (M, L, XL would all be using the wrong specs). We aren't using the bottom-to-text tokens, so that tipped me off that maybe we don't need the top-to-text.
…://github.com/adobe/spectrum-css into aramos-adobe/css1201-s2-slider-handle-variant
marissahuysentruyt
added
the
skip_vrt
There was a problem hiding this comment.
This is a big migration that was a ton of work, so I'm sorry I didn't get to finish looking through it on the first pass! For now I'm pointing out a few things I noticed after going over the first section in the token specs and will plan to pick up the rest a bit later!
components/slider/index.css
Outdated
| @@ -619,41 +697,67 @@ | |||
|
|
|||
| &:active, | |||
| &.is-dragged { | |||
| transform: perspective(var(--spectrum-slider-downstate-perspective)) translateZ(var(--spectrum-component-size-difference-down)); | |||
There was a problem hiding this comment.
It looks like this transform declaration might be a duplicate of one around L396? So far I'm not seeing a difference in how the .spectrum-Slider-handle selectors are scoped and it doesn't seem to change much if I leave it out. Does it seem like a duplicate to you?
And if so, is there any difference between the .spectrum-Slider-handle styles here and the ones that start around L372? Would it affect anything if we refactored a bit and combined those styles? For instance, it looks like border-width and border-style are in the upper (around L372) section, while border-color is in the lower (around L680) section, the code works the same, but those seem like they'd make more sense for a human to read through if they're physically closer to each other, unless there's a specific reason to keep them separated.
There was a problem hiding this comment.
@rise-erpelding Good eye on that. I didn't realize it the was same selector 🙈
|
|
||
| .spectrum-Slider-handle { | ||
| /* stylelint-disable-next-line spectrum-tools/no-unknown-custom-properties -- used to assign downstate perspective */ | ||
| --spectrum-slider-downstate-perspective: max(var(--spectrum-downstate-height), var(--spectrum-downstate-width) * var(--spectrum-component-size-width-ratio-down)); |
There was a problem hiding this comment.
A couple of things here:
-
Do we want
--spectrum-slider-downstate-perspectiveset twice?- I still haven't combed through the specs for Slider fully yet, so I could be wrong, but I'm wondering about this
--spectrum-slider-downstate-perspectivethat uses the calculated perspective vs. the one you've got in line 95 that uses--spectrum-component-size-minimum-perspective-down. Do we need both? - If we want to set it differently for small vs. other sizes, so far in the browser I'm not seeing that
--spectrum-slider-downstate-perspectiveis adjusting to use the minimum perspective token (--spectrum-component-size-minimum-perspective-down) for small size, which I'm guessing might be something about the specificity (this one's scoped to the handle, I think the other one is scoped to.spectrum-Slider).
- I still haven't combed through the specs for Slider fully yet, so I could be wrong, but I'm wondering about this
-
I'm also wondering though if we can get away with using the minimum perspective token instead of the full decorator?
- Referencing the documentation we have on this, I'm wondering if the slider handle is small enough at all sizes that we can use minimum perspective for this all the time instead of calculated perspective (I think with
:beforeit does go 2px over, technically). - So it'd be more like checkbox rather than like button, which would mean that we can drop the use of the down state decorator from the .stories.js file and that this
--spectrum-slider-downstate-perspectivecould just be set to the minimum perspective token (--spectrum-component-size-minimum-perspective-down) for all sizes instead. - Overall though, it won't look all that different picking one down state implementation over the other. Sticking with the decorator is probably ok. Or switching to the minimum perspective method is also probably ok, even if we have a couple of edge cases where it starts getting a couple px larger than the intended use case.
- Referencing the documentation we have on this, I'm wondering if the slider handle is small enough at all sizes that we can use minimum perspective for this all the time instead of calculated perspective (I think with
| values: { | ||
| table: { disable: true } | ||
| }, |
There was a problem hiding this comment.
Is values how we get the range variant? Is there a way to see the range variant from the controls? And if not, is that because of the complexity of showing it through controls?
There was a problem hiding this comment.
If we did range, we'd have to document how to use both offset and range in the values array control. If you select one of the variants you'd have to change the values to 10 and 20 for range and and -15 for offset. I'm open to documenting that in the description for visual demonstration, but is that ratchet?
| /** | ||
| * Large track height. | ||
| */ | ||
| export const TrackHeight = Template.bind({}); | ||
| TrackHeight.args = { | ||
| ...Default.args, | ||
| trackHeight: "large", | ||
| }; | ||
| TrackHeight.tags = ["!dev"]; | ||
| TrackHeight.parameters = { | ||
| chromatic: { | ||
| disableSnapshot: true, | ||
| }, | ||
| }; | ||
| TrackHeight.storyName = "Large track height"; | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Thanks for adding this! It'd be nice to see large track height in various sizes. Maybe here in the docs. Maybe in the tests too? (If you need a reference, Josh did sizing for multiple variants in Accordion, that's a method you could follow here.)
| testHeading: "No Label Precise", | ||
| variant: "filled", | ||
| label: "", | ||
| isPrecise: true |
There was a problem hiding this comment.
Gosh, lots of tests here, great coverage!
I mentioned over in .stories.js that we might add sizing for the large track height, wondering if it might also be useful to add sizing for the precise variant too since it looks like the handle height differs per size?
| @@ -140,47 +191,54 @@ | |||
| margin-block-start: 0; | |||
|
|
|||
| .spectrum-Slider-label { | |||
| margin-inline-end: var(--mod-slider-value-side-padding-inline, var(--spectrum-slider-value-side-padding-inline)); | |||
| margin-inline-end: var(--mod-slider-control-to-side-field-label, var(--spectrum-slider-control-to-side-field-label)); | |||
There was a problem hiding this comment.
I'm sure that I've picked the wrong place for this comment, but it looks like there's no minimum track width. This may be an edge case but we probably don't want it to be this small. Maybe we can add a little flex: 1 or other flex-grow/flex-shrink/flex-basis magic somewhere to keep the track from shrinking so much when the side label is long?
| --spectrum-slider-editable-field-inline-size-small: 56px; | ||
| --spectrum-slider-editable-field-inline-size-medium: 70px; | ||
| --spectrum-slider-editable-field-inline-size-large: 82px; | ||
| --spectrum-slider-editable-field-inline-size-extra-large: 94px; |
There was a problem hiding this comment.
Ooooh tell me more about these. I see they're not in the tokens spec, and it looks like the tokens are the same values for medium and large. Is this just taken from looking at the width of the slider text field at different sizes?
We'll need a changeset for tokens too!
There was a problem hiding this comment.
I got these sizes recommended by Jason. I wanted to know what the minimum/suggested sizes were for the editable fields.
| --spectrum-slider-handle-border-radius: var(--spectrum-slider-handle-size); | ||
|
|
||
| /* Textfield passthrough */ | ||
| --mod-textfield-width: var(--mod-slider-editable-field-inline-size, var(--spectrum-slider-editable-field-inline-size-medium)); |
There was a problem hiding this comment.
I'm seeing in this screen shot that the passthrough is correctly being set to 70px, but when I inspect it looks like it's 48.344px, any idea why this happens?
I also see the note that says:
Note: Text field will have slight alterations to it (set width to accommodate 6 numbers and removal of field label/help text), but references the text field component.
I can't fit 6 digits in there, which makes me think that this text field is a bit too short.
There was a problem hiding this comment.
Looks like I had to add flex: 1 to allow the inline size of the text width to be the suggested width
Description
CSS-1201
CSS-1202
S2 slider migration changes:
New tokens
--spectrum-slider-control-to-field-label-editable-extra-large--spectrum-slider-control-to-field-label-editable-large--spectrum-slider-control-to-field-label-editable-medium--spectrum-slider-control-to-field-label-editable-small--spectrum-slider-control-to-side-field-label-extra-large--spectrum-slider-control-to-side-field-label-large--spectrum-slider-control-to-side-field-label-medium--spectrum-slider-control-to-side-field-label-small--spectrum-slider-control-to-text-field-extra-large--spectrum-slider-control-to-text-field-large--spectrum-slider-control-to-text-field-medium--spectrum-slider-control-to-text-field-small--spectrum-slider-editable-control-to-field-label--spectrum-slider-editable-control-to-text-field--spectrum-slider-editable-field-inline-size-extra-large--spectrum-slider-editable-field-inline-size-large--spectrum-slider-editable-field-inline-size-medium--spectrum-slider-editable-field-inline-size-small--spectrum-slider-emphasized-track-fill-color--spectrum-slider-precision-handle-height--spectrum-slider-precision-handle-height-extra-large--spectrum-slider-precision-handle-height-large--spectrum-slider-precision-handle-height-medium--spectrum-slider-precision-handle-height-small--spectrum-slider-precision-handle-width--spectrum-slider-handle-extra-large--spectrum-slider-handle-large--spectrum-slider-handle-medium--spectrum-slider-handle-smallHow 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
- [ ] Precision handle for all shirt sizes
- [ ] Precision handle on slider variants (ramp, offset, range)
- [ ] Perspective onclick down on precision and default handle
- [ ] Emphasized track is visible
- [ ] Interaction states are visible for variants and embedded components
Regression testing
Validate:
Screenshots
To-do list