#1203 slider improving label positioning and width #1376
Conversation
ktabors
requested review from
dannify,
devongovett and
snowystinger
as code owners
ktabors
changed the title
|
|
||
| min-block-size: var(--spectrum-slider-height); | ||
| min-inline-size: var(--spectrum-slider-min-width); | ||
| min-inline-size: var(--spectrum-global-dimension-size-2400); |
There was a problem hiding this comment.
Should I create a PR to adjust the min-width to 192px here? https://git.corp.adobe.com/Spectrum/spectrum-dna/blob/main/data/elements/slider/SliderBase.mjs#L40
There was a problem hiding this comment.
Perhaps a variable in the :root of this file would suffice for now? Something like --spectrum-slider-min-height: var(--spectrum-global-dimension-size-2400);
There was a problem hiding this comment.
Ah never mind, my mistake. Just realized that --spectrum-slider-min-width is already a thing. Yeah I think opening a PR makes sense
There was a problem hiding this comment.
There was a problem hiding this comment.
Is it supposed to change the min width or just the default width?
There was a problem hiding this comment.
min width is default width, right?
|
Build successful! 🎉 |
There was a problem hiding this comment.
Is the default slider width of 192px supposed to be applied to outer slider div (aka spectrum-Slider) or the slider control (aka spectrum-Slider-controls)? Been trying to figure out which makes sense but the XD file and spectrum-css docs don't show anything being 192px by default
The answer to the above will affect things like the inline-size calculation the slider-control does and stuff
|
Discussed where the default width should be set and it should be on the track. This is how other labeled form components like TextField are done, with it on the input and not the container wrapping the label and the input. The custom width should go on the container. |
| font-feature-settings: "tnum"; | ||
| text-align: end; | ||
|
|
||
| &:only-child { |
There was a problem hiding this comment.
What's the :only-child for?
There was a problem hiding this comment.
When it is a side label, the value and label are in separate label containers... the way I added the start align to the value when side label is that value is the only child of a label container.
There was a problem hiding this comment.
Oh I assumed there was a class for the side label state but I guess not... 😞
Maybe just add a comment to explain what you just said then.
There was a problem hiding this comment.
Maybe we should use the spectrum-Slider--positionSide class now?
| let upperTrack = ( | ||
| <div | ||
| className={classNames(styles, 'spectrum-Slider-track')} | ||
| style={{[cssDirection]: `${state.getThumbPercent(1) * 100}%`, width: `${(1 - state.getThumbPercent(1)) * 100}%`}} /> |
There was a problem hiding this comment.
Ah this one I can answer: @ktabors had noticed the following issue with range sliders where the right most track line spills into the right thumb:
Looking at https://opensource.adobe.com/spectrum-css/slider.html#range, the right most track line only has width applied to it, the left/right inline style here was extraneous
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
|
||
| /* These calculations prevent the track from spilling outside of the control */ | ||
| inline-size: calc(100% - calc(var(--spectrum-slider-controls-margin) * 2)); | ||
| inline-size: calc(100% - calc(var(--spectrum-slider-controls-margin))); |
There was a problem hiding this comment.
Since the default width is being specified on the wrapping div now, I think you can revert this change here and remove the margin-inline-end that you added below
|
Build successful! 🎉 |
|
Build successful! 🎉 |
| touch-action: none; | ||
|
|
||
| width: var(--spectrum-global-dimension-size-2400); | ||
| min-inline-size: var(--spectrum-slider-min-width); |
There was a problem hiding this comment.
I thought textfield had the min width on the input parent not the field and input parent parent.
There was a problem hiding this comment.
Yeah honestly I don't know which is right. I am going to bring it up with Spectrum because I don't think I was specific enough when I asked them originally.
|
Build successful! 🎉 |
This should work now. Co-authored-by: Devon Govett <devongovett@gmail.com>
|
Build successful! 🎉 |
Fixes a few issues in #1203
Moves the label value to the edge when on top and align start when a side label.
Default width is 192px.
Changed the width on the width and label overflow stories.
✅ Pull Request Checklist:
📝 Test Instructions:
Test all the slider stories, specifically the value stories, the the label side, the label overflow, and the custom width stories.
🧢 Your Project:
RSP