Slider update #4614
Slider update #4614
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
tayyabataimur
force-pushed
the
slider-update
branch
2 times, most recently
from
1eee83a to
a2508f9
Compare
tayyabataimur
added
the
chromatic
github-actions
bot
removed
the
chromatic
tayyabataimur
added
the
chromatic
tayyabataimur
added
chromatic
github-actions
bot
removed
the
chromatic
tayyabataimur
added
the
chromatic
github-actions
bot
removed
the
chromatic
| const changeSpy = cy.stub().as("changeSpy"); | ||
| cy.mount(<Slider style={{ width: "400px" }} onChange={changeSpy} />); | ||
| cy.get(".saltSliderTrack").trigger("pointerdown", { | ||
| cy.get(".saltSliderTrack-rail").trigger("pointerdown", { | ||
| button: 0, | ||
| clientX: 50, | ||
| clientY: 50, | ||
| }); | ||
| cy.get("@changeSpy").should("have.callCount", 1); |
There was a problem hiding this comment.
We should test the correct values are returned when we test callbacks
| <SliderThumb | ||
| aria-describedby={formFieldDescribedBy} | ||
| aria-label={ariaLabel} | ||
| aria-labelledby={formFieldLabelledBy} |
There was a problem hiding this comment.
aria-labelledby and aria-describedby being passed in as props should also be handled (and tested)
| aria-labelledby={formFieldLabelledBy} | ||
| aria-valuemax={max} | ||
| aria-valuemin={min} | ||
| aria-valuetext={ariaValueText} |
There was a problem hiding this comment.
Should we be providing guidance on how to use this, or is this just to cover all bases? @jake-costa
| @@ -1,2 +1,3 @@ | |||
| export * from "./Slider"; | |||
| export * from "./types"; | |||
| export * from "./RangeSlider"; | |||
| export * from "./internal/useSliderThumb"; | |||
There was a problem hiding this comment.
We should be careful that this isn't exposing this from the package unnecessarily, import {} from "@salt-ds/core"
| transform: translateX(-50%); | ||
| cursor: pointer; | ||
| outline: none; | ||
| top: -5px; |
There was a problem hiding this comment.
Is this value correct? Or is there a token / calculation we can use
| use visibility: hidden as we need the screen | ||
| reader to announce it */ | ||
| .saltSliderThumb-accessibleText { | ||
| position: absolute; |
There was a problem hiding this comment.
I think we should use the standard approach here? There should be an example elsewhere in the code
There was a problem hiding this comment.
Found useAriaAnnouncer() which I think would suit the purpose
| transform: translate(-50%, -50%); | ||
| border: 1px solid var(--salt-accent-borderColor); | ||
| padding: var(--salt-spacing-50) var(--salt-spacing-100); | ||
| user-select: none; |
There was a problem hiding this comment.
We should check this behaviour, although I assume this is included to prevent selecting text when dragging. When not dragging should selecting text work? @jake-costa @navkaur76
Edit: I just checked, currently dragging the tooltip changes the value so I assume this was intentional
There was a problem hiding this comment.
@joshwooding Yep, this was intentional to prevent selecting the tooltip's text when dragging. Good question about being able to select the text when not dragging, keen to hear Jake and Nav's thoughts on this.
|
|
||
| .saltSliderTrack-minLabel, | ||
| .saltSliderTrack-maxLabel { | ||
| user-select: none; |
There was a problem hiding this comment.
We should probably just check if this is necessary everywhere
| } | ||
|
|
||
| .saltSliderTrack-minLabel { | ||
| order: 1; |
There was a problem hiding this comment.
I always get a bit nervous when I see order being used, due to accessibility worried @jake-costa maybe something to have a quick look at
| By default the minimum and maximum values are shown, and can be displayed either inline or below the track (using the `marks` prop to `"bottom"`). | ||
|
|
||
| Alternately, incremental marks can be shown along the track by setting the `marks` prop to `"all"`. The incremental marks will match the `step` prop value (i.e. for a `step` value of `5`, the marks will display as "0", "5", "10", "15", etc.) Note that when all marks are shown, they are always displayed below the track. | ||
| Selection of a single value from a horizontally oriented slider track. Values can be displayed either inline or below the track. |
| By default the minimum and maximum values are shown, and can be displayed either inline or below the track (using the `marks` prop to `"bottom"`). | ||
|
|
||
| Alternately, incremental marks can be shown along the track by setting the `marks` prop to `"all"`. The incremental marks will match the `step` prop value (i.e. for a `step` value of `5`, the marks will display as "0", "5", "10", "15", etc.) Note that when all marks are shown, they are always displayed below the track. | ||
| Selection of a single value from a horizontally oriented slider track. Values can be displayed either inline or below the track. |
There was a problem hiding this comment.
I can understand why but since we only support one orientation for now we don't need to add "horizontally oriented" everywhere
|
|
||
| ## With Markers | ||
|
|
||
| You can display markers under the values of your choice with custom labels. |
There was a problem hiding this comment.
This wording confused me a bit, "markers" are the labels so we might need to clear up the wording a bit
There was a problem hiding this comment.
So the way I think of this is, we always have min/max labels and if we want to add more labels, we pass them through markers or marks to add those labels at certain marks.
|
@tayyabataimur FYI for examples with a visible label should referenced by aria-labelledby on the slider element. |
SliderandRangeSlidercomponents.