-
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 tooltips for single and two handle sliders #15129
feat(slider): add tooltips for single and two handle sliders #15129
Conversation
✅ 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. |
Ahh, there are some legit test failures on account of markup and styles here that needed to shuffle around on account of the Tooltips. I'll take a look at those in the morning. |
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.
Code side of things looks pretty solid. The interaction when dragging is a bit odd though. The tooltip seems to disappear randomly. Also when you drag and get near the other handle, the other handle's tooltip appears on hover, which probably shouldn't happen.
two.handle.slider.dragging.interaction.mov
Would it make sense to "lock" the tooltip open (or closed) when dragging? @laurenmrice do you have any thoughts?
@m4olivei If you haven't already, could you take a look at how this impacts the way in which the component is read via screenreaders? I wonder if it might make sense to hide the tooltips from assistive tech? |
I also see there's some recent comments on the original issue questioning the approach #14549 (comment) |
See update in connected issue #14549 (comment) |
Nice catches!
This is a regression, I hadn't even checked the skeletons. Oops 😊 .
This is a bug. I'm not accounting for and locking the tooltip open, when the drag start begins with a track click, and not a direct click on the handle. Will work on these probably tomorrow, but maybe today if I can burn through some other tasks. |
Should be ready for another look. |
@m4olivei LGTM! 💯 Great work. Love that we were able to sneak in the tooltips. I'm going to let Carbon have final say on if this is good to go but I think it is. CC: @laurenmrice 😄 |
Looks good to me! Thanks @m4olivei ! |
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! 🥳
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.
🥳 🎉
Yay, approved! @tay1orjones noticed that we got booted from the merge queue. I'm unclear on why, do you have any insight? |
@m4olivei sorry, not sure. Sometimes the merge queue checks time out without reason. We're still figuring out the best settings for the merge queue. We got this in though just now 👍 |
Thanks @tay1orjones ! |
…design-system#15129) * fix(slider): refine what is focusable to be more broadly inclusive * feat(slider): add tooltips to slider * fix(slider): adjust for nice default tooltips on single handle * fix(slider): correct single handle RTL bug * fix(slider): put back the focus handler for the lower thumb * fix(slider): rtl positioning bug * chore: udpate tests * test(slider): adjust unit tests for slider w/ tooltips * fix(slider): remove space when text input is hidden * fix(slider): restore blue focus filled track * feat(slider): adjustments to tooltip positioning * feat(slider): use a ThumbWrapper component to conditionaly do tooltips * feat(slider): add stories for hidden inputs * fix(tooltip): hold the tooltip open if the user is mid-drag interaction * refactor(slider): move slider handle componets into own file * fix(slider): make skeletons good again * fix(slider): manage focus when activating using the track * chore(slider): remove redundant preventDefault * fix(slider): more robust activeHandle detection * test(slider): fix slider tests * fix(slider): fix positioning of thumbs for rtl * fix(slider): correct tooltips rtl --------- Co-authored-by: Andrea N. Cardona <[email protected]>
Closes #14549
Adds tooltips to the Slider componet handles. Currently present by default for both single handle and double handle. Subject to design approval.
Changelog
New
Testing / Reviewing