-
Notifications
You must be signed in to change notification settings - Fork 4
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(slider): create slider #1503
Conversation
Codecov Report
@@ Coverage Diff @@
## next #1503 +/- ##
==========================================
+ Coverage 91.91% 91.97% +0.06%
==========================================
Files 277 281 +4
Lines 4180 4264 +84
Branches 755 781 +26
==========================================
+ Hits 3842 3922 +80
- Misses 313 317 +4
Partials 25 25
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
size-limit report 📦
|
height: var(--slider-thumb-size); | ||
} | ||
.slider__input:focus { | ||
outline: none; |
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 removes the focus from the entire slider to focus just the thumb
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.
Is it worth writing a play
function that tabs onto the input so the focus is captured by chromatic or is that overkill?
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.
Ya, I think it's worth snapping!
Would it replace the default snap or should I make a new story for focus purpose?
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.
Hmm, can a person focus on the slider without focusing on the handle? how many tab presses does it take to go to the next focusable element? if it's more than one, we wouldn't want to remove this (necessarily... i'm not sure what best practice is in this case)
I recommend having any focus tests as separate from default, as the default state of any element isn't focused
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.
Found that the entirety of the slider is interactable, but made the hit box 44px high so it will be AAA compliant. Focus indicator remains on the thumb, however, and is snapped on chromatic
* Slider Input Track | ||
*/ | ||
/* Firefox */ | ||
.slider__input::-moz-range-track { |
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.
Main concern is using these pseudo elements
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.
May have to rely on consumer but will show recipes
- Could be tooltip
- Could be labels on ends
It would be nice to also have markers/tracks with labels on the sliders (not just the end). I have a few questions about how do recipes typically work for EDS.
- Are these just stories in
Slider.stories.tsx
are the stored elsewhere? - Are recipes exported from EDS or just purely examples to mimic in other repositories
I also agree that it would be nice to provide markers for the slider, but using the native list markers doesn't align with Regarding recipes:
|
/* Firefox */ | ||
.slider__input::-moz-range-track { | ||
/* fills left side of track as a percentage of the input value */ | ||
background: linear-gradient( |
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.
Would be nice to use accent-color
but to my knowledge only colors the filled track and is too modern for our browser policy
fyi @allisonbrownczi I think this PR is relevant for https://czi-tech.atlassian.net/browse/GST-108? |
@jinlee93 I can't seem to respond to specific comments, this is in regards to the tooltip vs markers. |
@allisonbrownczi I think in a week or two would have a better answer, but for now:
I can pair / discuss / feel free to bring up any thoughts / comments |
)} | ||
</div> | ||
); | ||
}; |
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.
Considered subcomponentizing the input and the markers but both need the css properties of the whole component to be styled properly
.slider__input { | ||
/* increases vertical hitbox for target size accessibility */ | ||
padding-top: 22px; | ||
padding-bottom: 22px; |
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.
Increased slider hitbox
src/components/Slider/Slider.tsx
Outdated
/** | ||
* List of markers to imply slider value. | ||
*/ | ||
markers?: string[]; |
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.
Currently constrained to strings (which could also incl emoji, for better or for worse) and evenly separates them out via justify-content: space-between
. Could consider expanding to ReactNode
if the need arises
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 think strings should be enough for our usecase at least
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 a little too open-ended at the moment. I think the intent is that the markers will be determined by the min
/max
/step
values, and the number of markers is mathematically determined by those three props when the display some arbitrary text (max - min / step
).
One way to account for this is to add checks to make sure markers is properly constrained. That would be useful whether they are simple strings or other symbols, or ReactNode[]
. spec tests to verify it works too.
Also, if the common case is that markers are always these numeric sets, we can add a marker utility function to generate the steps. For example, markers
is either string[]
or 'numeric'
(a special documented keyword), where we check for numeric
and generate the array values for consumers.
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 a little too open-ended at the moment. I think the intent is that the markers will be determined by the
min
/max
/step
values, and the number of markers is mathematically determined by those three props when the display some arbitrary text (max - min / step
).
I like this idea
Another idea is that we could update markers to be customizable markers?: Array<{ label?: string, value: number }>
instead of markers: Array<string>
, but then placing the actual markers may be get complicated
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.
Yea placing the markers according to the value of the marker (like how <option>
is done) does complicate things and make the prop a little more complex than I want. However, if the need arises for it in a product, will definitely be considered.
Andrew and I did pair on the generation of the markers which is now in this pr
@allisonbrownczi Added |
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.
We should also test what happens when the number of markers doesn't align with the min
/max
/step
values. Unlike using <datalist>
markers will only allow evenly-spaced marks
src/components/Slider/Slider.tsx
Outdated
/** | ||
* List of markers to imply slider value. | ||
*/ | ||
markers?: string[]; |
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 a little too open-ended at the moment. I think the intent is that the markers will be determined by the min
/max
/step
values, and the number of markers is mathematically determined by those three props when the display some arbitrary text (max - min / step
).
One way to account for this is to add checks to make sure markers is properly constrained. That would be useful whether they are simple strings or other symbols, or ReactNode[]
. spec tests to verify it works too.
Also, if the common case is that markers are always these numeric sets, we can add a marker utility function to generate the steps. For example, markers
is either string[]
or 'numeric'
(a special documented keyword), where we check for numeric
and generate the array values for consumers.
the intention right now is to only have evenly-spaced marks
I left it open like this since steps can be smaller than the differences between markers. E.g. a 0-100 control with steps of 1 but using markers to hint at current value |
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.
LGTM
src/components/Slider/Slider.tsx
Outdated
/** | ||
* Returns the lowest multiple of 10 that multiplies with all numbers in a list to make them integers. | ||
* Useful for floating point math. | ||
* @example | ||
* lowestTenMultiplier([-2.212, 0.1, 2.0, 100, 1000.01]) | ||
* // returns 1000 | ||
* @param numbers | ||
* @returns {number} Lowest multiple of 10. | ||
*/ | ||
const lowestTenMultiplier = (numbers: number[]): number => { | ||
let multiplier = 1; | ||
while ( | ||
numbers.some((number) => !Number.isInteger((number * multiplier) % 1)) | ||
) { | ||
multiplier *= 10; | ||
} | ||
return multiplier; | ||
}; |
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.
nit: we can also save this to src/utils and write separate unit tests for it, since this is a generic piece of functionality. The tests for this would serve as a good set of examples too.
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.
Moved to src/util/
and wrote some tests 👍
{label && ( | ||
<Label className={labelClassName} htmlFor={sliderId} text={label} /> | ||
)} | ||
<input |
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.
a11y: we might be able to add a meaningful aria-describedby
which formats a string describing the slider. This could be a fieldNode (which other inputs use), perhaps hidden by default or something. the fieldNote
would override this when specified.
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.
import { findLowestTenMultiplier } from './findLowestTenMultiplier'; | ||
|
||
describe('findLowestTenMultiplier', () => { | ||
describe('error throws', () => { |
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.
++
## [10.0.0](v9.1.0...v10.0.0) (2023-03-02) ### ⚠ BREAKING CHANGES * remove data-bootstrap-override EDS-850 ### Features * **Avatar:** add avatar component ([#1510](#1510)) ([bc21f85](bc21f85)) * **slider:** create slider ([#1503](#1503)) ([e7ced34](e7ced34)) * **TextareaField:** add TextArea base component and TextareaField ([#1493](#1493)) ([f2ba31d](f2ba31d)) ### Bug Fixes * remove data-bootstrap-override EDS-850 ([3b5d59b](3b5d59b)) * remove old, outdated, unnecessary snapshot ([fb63a34](fb63a34)) * **Select:** sync label design with form fields ([#1506](#1506)) ([efe9330](efe9330)) * update deps ([9a80e7f](9a80e7f)) * upgrade axe-core from 4.4.3 to 4.6.3 ([af3f9c5](af3f9c5))
[EDS-818]
Summary:
<input type="range">
elementTest Plan:
edu-stack
ortraject
as a sanity check if changes affect build or deploy, or are breaking, such as token changes, widely used component updates, hooks changes, and major dependency upgrades.