feat(Slider): add support for custom tooltip content#12531
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds optional ChangesSlider Custom Tooltip
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-core/src/components/Slider/Slider.tsx (1)
479-483: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a nullish check for
tooltipContent
tooltipContentis typed asReact.ReactNode, so0and''get ignored here and the tooltip falls back tofindAriaTextValue(). UsetooltipContent !== undefined(or??) instead of a truthiness check.Proposed fix
- content={tooltipContent ? tooltipContent : findAriaTextValue()} + content={tooltipContent !== undefined ? tooltipContent : findAriaTextValue()}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-core/src/components/Slider/Slider.tsx` around lines 479 - 483, The Tooltip content selection in Slider should not use a truthiness check for tooltipContent, since React.ReactNode can legitimately be 0 or an empty string. Update the Tooltip rendering logic in Slider to use a nullish check (for example, tooltipContent !== undefined or the nullish coalescing pattern) so Tooltip receives tooltipContent whenever it is defined, and only falls back to findAriaTextValue() when tooltipContent is actually undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/react-core/src/components/Slider/Slider.tsx`:
- Around line 479-483: The Tooltip content selection in Slider should not use a
truthiness check for tooltipContent, since React.ReactNode can legitimately be 0
or an empty string. Update the Tooltip rendering logic in Slider to use a
nullish check (for example, tooltipContent !== undefined or the nullish
coalescing pattern) so Tooltip receives tooltipContent whenever it is defined,
and only falls back to findAriaTextValue() when tooltipContent is actually
undefined.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e4240228-16d6-425e-9418-0b581c9cb8f4
📒 Files selected for processing (4)
packages/react-core/src/components/Slider/Slider.tsxpackages/react-core/src/components/Slider/__tests__/Slider.test.tsxpackages/react-core/src/components/Slider/examples/Slider.mdpackages/react-core/src/components/Slider/examples/SliderCustomTooltip.tsx
|
Preview: https://pf-react-pr-12531.surge.sh A11y report: https://pf-react-pr-12531-a11y.surge.sh |
| aria-valuetext={findAriaTextValue()} | ||
| aria-label={thumbAriaLabel} | ||
| aria-valuetext={thumbAriaValueText ? thumbAriaValueText : findAriaTextValue()} | ||
| aria-label={thumbAriaLabel ? thumbAriaLabel : thumbAriaLabel} |
There was a problem hiding this comment.
Did you mean to use a different value for one of these?
| aria-valuenow={localValue} | ||
| aria-valuetext={findAriaTextValue()} | ||
| aria-label={thumbAriaLabel} | ||
| aria-valuetext={thumbAriaValueText ? thumbAriaValueText : findAriaTextValue()} |
There was a problem hiding this comment.
Nit:
| aria-valuetext={thumbAriaValueText ? thumbAriaValueText : findAriaTextValue()} | |
| aria-valuetext={thumbAriaValueText ?? findAriaTextValue()} |
| triggerRef={thumbRef} | ||
| entryDelay={0} | ||
| content={findAriaTextValue()} | ||
| content={tooltipContent ? tooltipContent : findAriaTextValue()} |
| /** Current value of the slider. */ | ||
| value?: number; |
There was a problem hiding this comment.
Let's also add a tooltipProps to spread to the Tooltip
|
|
||
| ``` | ||
|
|
||
| ### Custom step tooltip |
There was a problem hiding this comment.
To add some example verbiage:
You can customize the content of the tooltip by passing the
tooltipContentproperty. By default this tooltip will act as a description to the slider thumb, and thus shouldn't include critical information about the current slider step unless that information is part of the step's aria-valuetext.If instead you want the tooltip to act as the human-readable value of the slider step - such as when all slider step labels are hidden - you must also pass the
thumbAriaValueTextproperty with the same string value as thetooltipContent. Additionally, you should pass the tooltip props object{aria: 'none', 'aria-live': 'off'}totooltipPropsin order to help prevent duplicate announcement from assistive technologies.
| <Slider | ||
| hasTooltipOverThumb | ||
| tooltipContent={customTooltipContent()} | ||
| value={value} | ||
| onChange={(_event: SliderOnChangeEvent, value: number) => setValue(value)} | ||
| customSteps={steps} | ||
| /> |
There was a problem hiding this comment.
May not be absolutely necessary with the above example verbiage added in, but we could add a second slider tothis example showing an implementation more similar to the original issue, where each step is a date timestamp. If we do, maybe we have the slider steps min and max values be 1-10 with whole number intervals
3 participants
What: Closes #12160
tooltipContent, which overrides the default behavior of the current value for a custom tooltipSummary by CodeRabbit
New Features
thumbAriaValueTextandtooltipContentto customize the slider thumb’saria-valuetextand the tooltip text.Documentation
Bug Fixes
Tests
aria-valuetextbehavior.