Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughSlider is migrated from radix-ui to Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@apps/www/src/content/docs/components/slider/index.mdx`:
- Line 14: Remove the incorrect named import SliderValue from the import listing
and keep only Slider (i.e., replace the import that references SliderValue with
an import that only includes Slider), then update any usages of the removed
symbol to reference the compound property Slider.Value instead of SliderValue so
the code uses the exported compound component correctly.
In `@apps/www/src/content/docs/components/slider/props.ts`:
- Around line 41-42: The onValueChange prop is currently typed with
eventDetails: any; update its signature to use the proper Base UI type by
replacing the any with Slider.Root.ChangeEventDetails from `@base-ui/react` so
consumers get correct typing for reason, native event, cancel/allowPropagation;
modify the declaration of onValueChange?: (value: number | number[],
eventDetails: any) => void to onValueChange?: (value: number | number[],
eventDetails: Slider.Root.ChangeEventDetails) => void and add the necessary
import if missing.
In `@packages/raystack/components/slider/__tests__/slider.test.tsx`:
- Around line 122-136: The tests were weakened to accept a null aria-label,
masking accessibility regressions; update the Slider component to always provide
accessible defaults: when rendering the root element with class styles.slider
inside the Slider component (handling prop variant === 'range' vs default
single), set an explicit aria-label if none is passed (e.g., "Slider" for single
and "Range slider" for range) or add defaultProps/parameter defaults so the root
always has the correct aria-label; update the rendering logic that produces the
root element to prefer an explicit aria-label prop over Base UI defaults.
- Around line 144-152: The test "sets aria-valuetext" no longer asserts the
aria-valuetext value; update the test in slider.test.tsx (the it block named
'sets aria-valuetext') to make a meaningful assertion: render the Slider
component with value={50} and aria-valuetext='50 percent', then grab the element
via screen.getByRole('slider') and assert the attribute is forwarded (e.g.,
expect(slider).toHaveAttribute('aria-valuetext', '50 percent') or
expect(slider.getAttribute('aria-valuetext')).toBe('50 percent')). If Base UI
transforms aria text via a getAriaValueText callback, instead adjust the Slider
component to accept and forward aria-valuetext (or provide a getAriaValueText
prop) so the test can verify the effective aria text.
🧹 Nitpick comments (5)
packages/raystack/components/slider/__tests__/slider.test.tsx (2)
163-183: Avoid nesting async operations insidewaitFor.Placing
actanduser.keyboardinsidewaitForis an anti-pattern.waitForis for polling assertions, not for triggering actions. The 1000ms timeout suggests potential test flakiness.♻️ Recommended test structure
it('calls onValueChange with single value', async () => { const user = userEvent.setup(); const handleChange = vi.fn(); - const { container } = render( + render( <Slider onValueChange={handleChange} defaultValue={50} /> ); - await waitFor(async () => { - const input = container.querySelector( - 'input[type="range"]' - ) as HTMLInputElement; - expect(input).toBeInTheDocument(); - - if (input) { - await act(async () => { - input.focus(); - await user.keyboard('{ArrowRight}'); - }); - } - }); + const slider = screen.getByRole('slider'); + await user.click(slider); + await user.keyboard('{ArrowRight}'); - // Give Base UI time to process the change - await waitFor( - () => { - expect(handleChange).toHaveBeenCalled(); - }, - { timeout: 1000 } - ); + await waitFor(() => { + expect(handleChange).toHaveBeenCalled(); + }); const callArgs = handleChange.mock.calls[0]; - // Base UI passes value as first arg, eventDetails as second expect( typeof callArgs[0] === 'number' || Array.isArray(callArgs[0]) ).toBe(true); });
203-220: Same anti-pattern: async operations insidewaitForwith long timeout.Apply the same refactoring as suggested for the single value test.
packages/raystack/components/slider/slider.module.css (1)
65-65: Note: Sub-pixel borders may render inconsistently.
border: 0.5pxmay not render consistently across all browsers and display densities. Consider using1pxfor more predictable rendering, or verify the visual appearance across target browsers.Also applies to: 86-86
packages/raystack/components/slider/slider.tsx (2)
39-46: Consider cachinggetLabelresults to avoid redundant calls.
getLabel(i)is called multiple times per thumb render (lines 66, 79, 85). While the function is memoized withuseCallback, the result isn't cached.💡 Suggested optimization
- const getLabel = useCallback( - (index: number) => { - if (!label) return undefined; - if (typeof label === 'string') return label; - return label[index]; - }, - [label] - ); + const labels = useMemo(() => { + if (!label) return [undefined, undefined]; + if (typeof label === 'string') return [label, label]; + return label; + }, [label]);Then use
labels[i]directly in the render.
50-56: Consider adding defaultaria-labelfor accessibility.The component doesn't set a default
aria-labelon the root element. This caused the accessibility tests to be weakened to acceptnull. For better accessibility, provide meaningful defaults.♻️ Suggested fix
return ( <SliderPrimitive.Root ref={ref} className={slider({ variant, className })} thumbAlignment='edge' + aria-label={props['aria-label'] ?? (isRange ? 'Range slider' : 'Slider')} {...props} >Note: Place
aria-labelbefore{...props}to allow user override.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/raystack/components/slider/__tests__/slider.test.tsx`:
- Around line 73-77: The test "sets step value" currently only checks the input
exists; update it to assert the step attribute is applied by grabbing the range
input (using the existing container.querySelector('input[type="range"]')) and
asserting its step is "5" (e.g., expect(slider).toHaveAttribute('step', '5') or
expect(slider.getAttribute('step')).toBe('5')) after rendering <Slider step={5}
/> so the Slider component's step prop is actually verified.
- Around line 177-186: The test currently performs interactions inside waitFor
(the block using waitFor with lowerSlider.focus() and await
user.keyboard('{ArrowRight}')), which is an anti-pattern; move the interactions
(focus and user.keyboard wrapped in act or using userEvent) outside of waitFor
and reserve waitFor for assertions only. Concretely, call lowerSlider.focus()
and await user.keyboard('{ArrowRight}') (inside act if your test harness
requires) before invoking waitFor, then inside waitFor query inputs and assert
expected outcomes; reference the existing symbols waitFor, act, lowerSlider, and
user.keyboard when making the change.
- Around line 137-149: The test uses an async callback inside waitFor which is
an anti-pattern; change the sequence so waitFor only asserts the input exists
(use waitFor(() =>
expect(container.querySelector('input[type="range"]')).toBeInTheDocument()) or
similar) and then perform the interactions outside of waitFor: locate the input
via container.querySelector (or the same selector used now), call input.focus()
and then wrap user.keyboard('{ArrowRight}') inside act if needed; update
references to waitFor, input.focus, act, and user.keyboard so the interaction
steps happen after the waitFor assertion completes.
🧹 Nitpick comments (2)
apps/www/src/content/docs/components/slider/index.mdx (1)
40-42: Restore the Accessibility documentation section.The Accessibility section (including WAI-ARIA guidelines reference, ARIA attributes documentation, custom ARIA label examples, and screen reader considerations) was removed from this file. While Base UI's Slider handles accessibility attributes automatically, the documentation provided valuable guidance for component consumers on:
- ARIA attributes the component manages (
aria-label,aria-valuenow,aria-valuemin,aria-valuemax)- How to provide custom accessibility labels
- Screen reader behavior and keyboard interaction support
Consider restoring this section to maintain comprehensive documentation for developers implementing this component.
packages/raystack/components/slider/__tests__/slider.test.tsx (1)
159-163: Tighten the assertion to match the single-value slider's expected type.The current assertion accepts both
numberand array, which weakens the test's ability to catch regressions. Since this test initializes the slider withdefaultValue={50}(a number), theonValueChangecallback should receive a number specifically, consistent with the Base UI API's handling of single-value vs. range sliders.💡 Consider tightening the assertion
const callArgs = handleChange.mock.calls[0]; - // Base UI passes value as first arg, eventDetails as second - expect( - typeof callArgs[0] === 'number' || Array.isArray(callArgs[0]) - ).toBe(true); + // Single-value slider should pass a number + expect(typeof callArgs[0] === 'number').toBe(true);
2 participants
Description
Breaking Changes
Changes
Docs & Tests
Summary by CodeRabbit
New Features
Breaking Changes
Documentation