-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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): add range slider #14297
feat(slider): add range slider #14297
Conversation
7d97253
to
f087b65
Compare
✅ Deploy Preview for carbon-components-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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 awesome! Playing around with it some I see a few nits but they're things already on your list to address (bound events).
If you think it would be helpful, this component is due to be converted from a class-based component to a functional component. If you're interested, this could be a big improvement to readability of the component being able to encapsulate some of the logic into useEffect
and useLayoutEffect
blocks. No sweat if not, but thought I'd mention it.
Thanks!
I'm glad you brought that up! I wondered if you all had any such initiative. I'll try to include that as well here as time allows. |
448575a
to
332b5fb
Compare
f20ebab
to
8fef060
Compare
I think you're spot on with your suggestion of a background color @m4olivei, I think it might need a small tweak. With the addition of a background-color on the entire icon, it leads to a middle indicator that hides and appears arbitrarily. In addition, the background on both sides creates a second line-segment.
Instead, I think we could have the background-color only appear on the rail directly inside the icon area on the right-hand side. That way, the middle indicator will always be visible, the rail isn't split into two line-segments, and makes it easier to read what is actually being selected. The same treatment would be provided to the focus state. The color used here would likely be |
Yep, makes sense @KevinCamelo . I've implemented that. Take a look. Be sure as well to jump between themes and check RTL to make sure I covered us in those use cases. |
Forgot to tag @laurenmrice . I think we've addressed the feedback from your last comment. Could you take a look as well? Let me know if you need any adjustments. If all is well Percy needs to be approved for the visual tweaks we've made with the last few commits. |
Looks good! I entered in a letter in an input just to see what would happen and got a placeholder error. Wasn't sure whether that gets resolved elsewhere or we need to have all error messages composed as part of this. I was looking at https://deploy-preview-14297--v11-carbon-react.netlify.app/?path=/story/components-slider--two-handle-slider#storybook-preview-wrapper |
Actually, one other thing I noticed. The slider label is coded as a label, but it doesn't look like it's associated with the two inputs. |
Looking great, Matthew. Went across all themes— just noting that the selected state seems to not have the correct color across themes. The selected rail should use the G10/White G90/100 (incorrect color on rail and icon) To match the dark theme of the OG slider: For the rail: Additional info on the icon: From what I can tell, in the base slider component instead of using the |
Thanks @mbgower !
That gets handled by the user passing a prop,
Yep. I struggled with this earlier in the development. What I settled on, was that since the single label can't pair to both inputs, I leave the label disconnected (I guess it could be swapped for a
I've corrected the casing on that. I kind of want to avoid adopting price for the example, b/c of internationalization, but open to update if you like. |
Thanks @KevinCamelo , nice catch. I've gone through and corrected those tokens. |
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.
Looks good, thank you @m4olivei ⭐️
b102902
Closes #14567
Enhancement to the current React Slider component, allowing for a range selection of two values.
Requirements: https://ibm.ent.box.com/notes/1252927738733?s=4s8h59gazpuxv0fyxb1xq1pcxx6qvcwl
Designs: https://www.figma.com/file/Jk3pWNBG6ZWIRoldq2tM1x/Range---Slider-Enhancement?type=design&node-id=425-22406&mode=design&t=7ah75PWKFPtNMLUj-0
Mostly done and ready for review. Things on my radar still to do:
Rewrite Slider component to hooks, Will happen in a follow up PRChangelog
New
ariaLabelInputUpper
optional prop for the Slider component to use as thearia-label
attribute for the upper inputnameUpper
optional prop for the Slider component that serves as thename
attribute for the upper inputvalueUpper
optional prop for the Slider component that serves as the default value for the upper value of the rangetwoHandles
prop for the SliderSkeleton component which enhances the SliderSkeleton to be a range slider skeletonChanged
valueUpper
prop is set, all ofvalue
,name
andariaLabelInput
props are applied to the lower bound.onBlur
callback function will provide ahandlePosition: HandlePosition
key in it'sdata
argument to indicate which input / handle was changed to the givenvalue
.onChange
callback function will providevalue
andvalueUpper
for when the Slider has two handles.value
refers to the lower handle value when the Slider has two handles.onRelease
callback function will providevalue
andvalueUpper
for when the Slider has two handles.value
refers to the lower handle value when the Slider has two handles.Testing / Reviewing
Extensive changes were made to the Slider component to accomodate this feature. Thourough regression testing should be done for the Slider with a single handle.
The Slider > Playground story is the best place for testing various Slider configurations (single and two handle variations).
valueUpper
. This effectively puts the slider in two handle mode.readOnly
. Slider should hide the handles and show the inputs as Read Only. Compare the visual style of the inputs to the Text Input component.Finally, the Slider Skeleton component has also been updated to support a
twoHandles
boolean prop.