Open
Conversation
Adds a new NumberField component wrapping @base-ui/react NumberField with Apsara design tokens. Supports standalone usage (<NumberField />) and composed usage with sub-components (Group, Input, Decrement, Increment, ScrubArea). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughA new NumberField component is added to the Raystack library, including implementation with Radix UI primitives, CSS styling, TypeScript type definitions, comprehensive test coverage, documentation with multiple demo configurations, and playground examples. The component is exported to the public API. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
packages/raystack/index.tsx (1)
47-47: Consider exportingNumberFieldScrubAreaPropstype for consumer convenience.The
number-field/index.tsxexportsNumberFieldScrubAreaProps, but it's not re-exported here. Consumers using composed NumberField with ScrubArea in TypeScript may need this type. Other composite components likeDataTableandThemeProviderexport their prop types at the top level.♻️ Proposed change
-export { NumberField } from './components/number-field'; +export { NumberField, type NumberFieldScrubAreaProps } from './components/number-field';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/index.tsx` at line 47, Top-level index is exporting NumberField but not the associated type NumberFieldScrubAreaProps; update the export list in this module to re-export NumberFieldScrubAreaProps (the type exported from ./components/number-field) so consumers can import the prop type from the package root alongside NumberField.packages/raystack/components/number-field/__tests__/number-field.test.tsx (3)
54-69: Button selection relies on DOM order, which is brittle.The tests assume
buttons[0]is decrement andbuttons[1]is increment. If the component's internal DOM structure changes, these tests will break silently (testing the wrong button) or fail unexpectedly.Consider using accessible names or test IDs for more robust selection:
♻️ Example using accessible names
it('increments value on increment button click', () => { const onValueChange = vi.fn(); render(<NumberField defaultValue={5} onValueChange={onValueChange} />); - const buttons = screen.getAllByRole('button'); - fireEvent.click(buttons[1]); + const incrementButton = screen.getByRole('button', { name: /increment/i }); + fireEvent.click(incrementButton); expect(onValueChange).toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/number-field/__tests__/number-field.test.tsx` around lines 54 - 69, The tests in number-field.test.tsx rely on button index order (buttons[0]/buttons[1]) which is brittle; update the two interaction tests for the NumberField component to select buttons by accessible name or explicit test id instead (e.g., use screen.getByRole('button', { name: /increment/i }) and screen.getByRole('button', { name: /decrement/i }) or screen.getByTestId('increment-button')/('decrement-button')), and if the component lacks accessible labels add appropriate aria-labels or data-testid attributes to the increment/decrement controls in NumberField so the tests target the intended buttons reliably.
99-105: Disabled state test only verifies input, not buttons.The test confirms the input is disabled but doesn't verify that increment/decrement buttons are also disabled or non-interactive.
♻️ Enhanced test covering all controls
it('disables all controls when disabled', () => { render(<NumberField defaultValue={0} disabled />); const input = screen.getByRole('textbox'); + const buttons = screen.getAllByRole('button'); expect(input).toHaveAttribute('disabled'); + buttons.forEach(button => { + expect(button).toHaveAttribute('data-disabled'); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/number-field/__tests__/number-field.test.tsx` around lines 99 - 105, The Disabled State test in number-field.test.tsx only checks the textbox but should also assert the increment/decrement controls are disabled; update the test that renders <NumberField defaultValue={0} disabled /> to locate the control buttons (e.g., by role 'button' or by their aria-labels used in NumberField) and assert they are disabled or not interactive (use toBeDisabled or ensure clicking does not change value), referencing the NumberField component's increment/decrement button elements to locate and verify both controls.
88-97: Test only verifies max boundary, missing min boundary coverage.The "respects min and max" test only attempts to increment beyond max. Consider adding coverage for decrementing below min to ensure both boundaries are enforced.
♻️ Enhanced test covering both boundaries
it('respects min and max', () => { - render(<NumberField defaultValue={10} min={0} max={10} />); + render(<NumberField defaultValue={0} min={0} max={10} />); const buttons = screen.getAllByRole('button'); const input = screen.getByRole('textbox'); + // Try to decrement below min + fireEvent.click(buttons[0]); + expect(input).toHaveValue('0'); + // Set to max and try to increment beyond + fireEvent.clear(input); + fireEvent.change(input, { target: { value: '10' } }); // Try to increment beyond max fireEvent.click(buttons[1]); expect(input).toHaveValue('10'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/number-field/__tests__/number-field.test.tsx` around lines 88 - 97, Extend the "respects min and max" test in NumberField to also verify the min boundary: after the existing max check, render (or re-render) <NumberField defaultValue={0} min={0} max={10} /> (or set defaultValue to min) then get the controls (buttons[0] is the decrement button, input via getByRole('textbox')), fireEvent.click(buttons[0]) to attempt to decrement below min and assert the input value remains '0'; reference the NumberField test case name "respects min and max", the buttons array (buttons[0] for decrement, buttons[1] for increment), and the textbox input when adding these expectations.packages/raystack/components/number-field/number-field.module.css (1)
70-77: Cursor style doesn't account for vertical scrub direction.The
.scrub-areausescursor: ew-resize(horizontal), but if thedirection="vertical"prop is actually supported (as documented in props.ts), the cursor should bens-resizefor vertical scrubbing.If vertical direction is supported, this may need to be handled via a data attribute or separate class:
♻️ Example approach for direction-aware cursor
.scrub-area { display: flex; cursor: ew-resize; } .scrub-area[data-orientation='vertical'] { cursor: ns-resize; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/number-field/number-field.module.css` around lines 70 - 77, The CSS currently hardcodes a horizontal resize cursor on .scrub-area and .scrub-area-label; update styling to be direction-aware by adding a vertical cursor variant (e.g., via a data attribute or separate class) and ensure the component that reads the direction prop in props.ts sets that attribute/class on the scrub container (referencing the .scrub-area and .scrub-area-label selectors and the direction prop). For example, add a selector for the vertical case (e.g., .scrub-area[data-orientation="vertical"] or .scrub-area.vertical) to use ns-resize and ensure the render logic of the NumberField (or the component that uses direction from props.ts) applies the attribute/class when direction === "vertical".apps/www/src/content/docs/components/number-field/demo.ts (1)
11-16: Consider using'number'control type for numeric props.The
min,max, andstepprops are numeric values, but the controls use'text'type. Using'number'type would provide better input validation and UX in the playground.♻️ Proposed refactor
controls: { defaultValue: { type: 'text', initialValue: '0' }, - min: { type: 'text', initialValue: '' }, - max: { type: 'text', initialValue: '' }, - step: { type: 'text', initialValue: '1' }, + min: { type: 'number', initialValue: undefined }, + max: { type: 'number', initialValue: undefined }, + step: { type: 'number', initialValue: 1 }, disabled: { type: 'checkbox', initialValue: false, defaultValue: false } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/www/src/content/docs/components/number-field/demo.ts` around lines 11 - 16, Update the controls configuration so numeric props use the 'number' control type: locate the controls object in demo.ts and change the control types for the min, max, and step entries (currently min, max, step) from 'text' to 'number', and adjust their initialValue/defaultValue to numeric values or empty/undefined as appropriate to match the new type and avoid string inputs in the NumberField playground.packages/raystack/components/number-field/number-field.tsx (2)
104-127: Consider adding a development warning when context is missing.Line 116 uses
context?.fieldIdwhich silently producesundefinedifScrubAreais rendered outside aNumberField. While this is an unlikely misuse, a dev-mode warning would help catch such errors early.♻️ Optional: Add dev warning
const NumberFieldScrubArea = forwardRef< ElementRef<typeof NumberFieldPrimitive.ScrubArea>, NumberFieldScrubAreaProps >(({ className, label, ...props }, ref) => { const context = useContext(NumberFieldContext); + if (process.env.NODE_ENV !== 'production' && !context) { + console.warn('NumberField.ScrubArea must be used within a NumberField'); + } + return (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/number-field/number-field.tsx` around lines 104 - 127, NumberFieldScrubArea silently uses context?.fieldId which hides misuses when rendered outside a NumberField; add a development-only warning: inside the NumberFieldScrubArea function (before returning) check if NumberFieldContext (the value from useContext(NumberFieldContext)) is falsy and, when process.env.NODE_ENV !== 'production', emit a clear console.warn (or use the project's dev invariant util) mentioning "NumberField.ScrubArea must be used within NumberField" and include guidance to provide NumberFieldContext/fieldId; keep behavior unchanged in production.
129-144: Hardcoded fill/stroke colors may conflict with theming.The
fill='black'andstroke='white'are hardcoded, which may not work well in dark mode. Consider using CSS variables orcurrentColorfor theme compatibility.♻️ Optional: Use CSS variables
<svg aria-hidden='true' - fill='black' + fill='var(--rs-color-foreground-primary, black)' height='14' - stroke='white' + stroke='var(--rs-color-background-primary, white)' viewBox='0 0 24 14'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/number-field/number-field.tsx` around lines 129 - 144, The SVG in CursorGrowIcon hardcodes fill='black' and stroke='white', which breaks theming; update CursorGrowIcon to remove hardcoded colors and use theme-aware values (e.g., set fill and/or stroke to "currentColor" or a CSS variable like var(--color-text)), and ensure any parent element sets the desired color via CSS so the icon respects light/dark mode and theming.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/www/src/content/docs/components/number-field/demo.ts`:
- Around line 5-7: The getCode function signature currently accepts a single
parameter but must match the GetCodeType interface by accepting two parameters
(updatedProps: Record<string, any>, props: Record<string, any>); change export
const getCode to (updatedProps, props) and use updatedProps when building the
props string (i.e., pass updatedProps to getPropsString) so the playground
receives the updated properties while preserving any use of props elsewhere in
the function.
In `@apps/www/src/content/docs/components/number-field/index.mdx`:
- Around line 117-123: The Accessibility doc incorrectly claims support for
aria-roledescription="Number field" and
aria-valuemin/aria-valuemax/aria-valuenow even though the component only applies
role="textbox"; remove the lines that reference these unsupported ARIA
attributes from the Accessibility section (i.e., delete the bullets mentioning
aria-roledescription and aria-valuemin/aria-valuemax/aria-valuenow) or
alternatively implement those attributes on the input if they are
intended—ensure any change targets the Accessibility section text that currently
lists role="textbox" and the ARIA attributes.
---
Nitpick comments:
In `@apps/www/src/content/docs/components/number-field/demo.ts`:
- Around line 11-16: Update the controls configuration so numeric props use the
'number' control type: locate the controls object in demo.ts and change the
control types for the min, max, and step entries (currently min, max, step) from
'text' to 'number', and adjust their initialValue/defaultValue to numeric values
or empty/undefined as appropriate to match the new type and avoid string inputs
in the NumberField playground.
In `@packages/raystack/components/number-field/__tests__/number-field.test.tsx`:
- Around line 54-69: The tests in number-field.test.tsx rely on button index
order (buttons[0]/buttons[1]) which is brittle; update the two interaction tests
for the NumberField component to select buttons by accessible name or explicit
test id instead (e.g., use screen.getByRole('button', { name: /increment/i })
and screen.getByRole('button', { name: /decrement/i }) or
screen.getByTestId('increment-button')/('decrement-button')), and if the
component lacks accessible labels add appropriate aria-labels or data-testid
attributes to the increment/decrement controls in NumberField so the tests
target the intended buttons reliably.
- Around line 99-105: The Disabled State test in number-field.test.tsx only
checks the textbox but should also assert the increment/decrement controls are
disabled; update the test that renders <NumberField defaultValue={0} disabled />
to locate the control buttons (e.g., by role 'button' or by their aria-labels
used in NumberField) and assert they are disabled or not interactive (use
toBeDisabled or ensure clicking does not change value), referencing the
NumberField component's increment/decrement button elements to locate and verify
both controls.
- Around line 88-97: Extend the "respects min and max" test in NumberField to
also verify the min boundary: after the existing max check, render (or
re-render) <NumberField defaultValue={0} min={0} max={10} /> (or set
defaultValue to min) then get the controls (buttons[0] is the decrement button,
input via getByRole('textbox')), fireEvent.click(buttons[0]) to attempt to
decrement below min and assert the input value remains '0'; reference the
NumberField test case name "respects min and max", the buttons array (buttons[0]
for decrement, buttons[1] for increment), and the textbox input when adding
these expectations.
In `@packages/raystack/components/number-field/number-field.module.css`:
- Around line 70-77: The CSS currently hardcodes a horizontal resize cursor on
.scrub-area and .scrub-area-label; update styling to be direction-aware by
adding a vertical cursor variant (e.g., via a data attribute or separate class)
and ensure the component that reads the direction prop in props.ts sets that
attribute/class on the scrub container (referencing the .scrub-area and
.scrub-area-label selectors and the direction prop). For example, add a selector
for the vertical case (e.g., .scrub-area[data-orientation="vertical"] or
.scrub-area.vertical) to use ns-resize and ensure the render logic of the
NumberField (or the component that uses direction from props.ts) applies the
attribute/class when direction === "vertical".
In `@packages/raystack/components/number-field/number-field.tsx`:
- Around line 104-127: NumberFieldScrubArea silently uses context?.fieldId which
hides misuses when rendered outside a NumberField; add a development-only
warning: inside the NumberFieldScrubArea function (before returning) check if
NumberFieldContext (the value from useContext(NumberFieldContext)) is falsy and,
when process.env.NODE_ENV !== 'production', emit a clear console.warn (or use
the project's dev invariant util) mentioning "NumberField.ScrubArea must be used
within NumberField" and include guidance to provide NumberFieldContext/fieldId;
keep behavior unchanged in production.
- Around line 129-144: The SVG in CursorGrowIcon hardcodes fill='black' and
stroke='white', which breaks theming; update CursorGrowIcon to remove hardcoded
colors and use theme-aware values (e.g., set fill and/or stroke to
"currentColor" or a CSS variable like var(--color-text)), and ensure any parent
element sets the desired color via CSS so the icon respects light/dark mode and
theming.
In `@packages/raystack/index.tsx`:
- Line 47: Top-level index is exporting NumberField but not the associated type
NumberFieldScrubAreaProps; update the export list in this module to re-export
NumberFieldScrubAreaProps (the type exported from ./components/number-field) so
consumers can import the prop type from the package root alongside NumberField.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4b8d0e3-aada-41fa-9e30-b5e4037d1981
📒 Files selected for processing (10)
apps/www/src/components/playground/index.tsapps/www/src/components/playground/number-field-examples.tsxapps/www/src/content/docs/components/number-field/demo.tsapps/www/src/content/docs/components/number-field/index.mdxapps/www/src/content/docs/components/number-field/props.tspackages/raystack/components/number-field/__tests__/number-field.test.tsxpackages/raystack/components/number-field/index.tsxpackages/raystack/components/number-field/number-field.module.csspackages/raystack/components/number-field/number-field.tsxpackages/raystack/index.tsx
Comment on lines
+5
to
+7
| export const getCode = (props: any) => { | ||
| return `<NumberField${getPropsString(props)}/>`; | ||
| }; |
Contributor
There was a problem hiding this comment.
getCode signature does not match the required GetCodeType interface.
According to apps/www/src/components/demo/types.ts, GetCodeType requires two parameters: (updatedProps: Record<string, any>, props: Record<string, any>). The current implementation only accepts one parameter, which will cause the playground to malfunction when generating code snippets.
🐛 Proposed fix
-export const getCode = (props: any) => {
- return `<NumberField${getPropsString(props)}/>`;
+export const getCode = (updatedProps: Record<string, any>, props: Record<string, any>) => {
+ return `<NumberField${getPropsString(updatedProps)}/>`;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export const getCode = (props: any) => { | |
| return `<NumberField${getPropsString(props)}/>`; | |
| }; | |
| export const getCode = (updatedProps: Record<string, any>, props: Record<string, any>) => { | |
| return `<NumberField${getPropsString(updatedProps)}/>`; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/www/src/content/docs/components/number-field/demo.ts` around lines 5 -
7, The getCode function signature currently accepts a single parameter but must
match the GetCodeType interface by accepting two parameters (updatedProps:
Record<string, any>, props: Record<string, any>); change export const getCode to
(updatedProps, props) and use updatedProps when building the props string (i.e.,
pass updatedProps to getPropsString) so the playground receives the updated
properties while preserving any use of props elsewhere in the function.
Comment on lines
+117
to
+123
| ## Accessibility | ||
|
|
||
| - The input has `role="textbox"` with `aria-roledescription="Number field"` | ||
| - Increment and decrement buttons are keyboard accessible | ||
| - ScrubArea associates its label with the input via `htmlFor` | ||
| - Supports `aria-valuemin`, `aria-valuemax`, and `aria-valuenow` | ||
| - Keyboard: Arrow Up/Down to increment/decrement, Shift for large step, Meta for small step |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the actual role and ARIA attributes in the component implementation
rg -n "role|aria-" packages/raystack/components/number-field/
# Check tests to see what role is asserted
rg -n "getByRole" packages/raystack/components/number-field/__tests__/Repository: raystack/apsara
Length of output: 1332
🏁 Script executed:
#!/bin/bash
# Search for aria-value attributes specifically
rg -n "aria-value" packages/raystack/components/number-field/
# Also check the entire number-field component file
wc -l packages/raystack/components/number-field/number-field.tsxRepository: raystack/apsara
Length of output: 121
🏁 Script executed:
cat -n packages/raystack/components/number-field/number-field.tsxRepository: raystack/apsara
Length of output: 5531
🏁 Script executed:
# Check the MDX file to see the exact content at lines 117-123
sed -n '117,123p' apps/www/src/content/docs/components/number-field/index.mdxRepository: raystack/apsara
Length of output: 429
🏁 Script executed:
# Check if aria-roledescription is mentioned anywhere in the component
rg -n "aria-roledescription" packages/raystack/components/number-field/
# Check Base UI documentation or types to see what ARIA attributes are supported
rg -n "aria-roledescription" --type ts --type tsxRepository: raystack/apsara
Length of output: 86
🏁 Script executed:
# Check if aria-roledescription is mentioned anywhere in the component
rg -n "aria-roledescription"Repository: raystack/apsara
Length of output: 198
Remove unsupported ARIA attributes from accessibility documentation.
The documentation claims the input supports aria-roledescription="Number field" and aria-valuemin, aria-valuemax, aria-valuenow, but the component does not implement any of these attributes. Only role="textbox" is actually applied. Remove lines referencing the unsupported ARIA attributes from the accessibility section, or update the component implementation to add them if they are intended.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/www/src/content/docs/components/number-field/index.mdx` around lines 117
- 123, The Accessibility doc incorrectly claims support for
aria-roledescription="Number field" and
aria-valuemin/aria-valuemax/aria-valuenow even though the component only applies
role="textbox"; remove the lines that reference these unsupported ARIA
attributes from the Accessibility section (i.e., delete the bullets mentioning
aria-roledescription and aria-valuemin/aria-valuemax/aria-valuenow) or
alternatively implement those attributes on the input if they are
intended—ensure any change targets the Accessibility section text that currently
lists role="textbox" and the ARIA attributes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
NumberFieldcomponent wrapping the Base UINumberFieldprimitive<NumberField />) which renders decrement, input, and increment internallyGroup,Input,Decrement,Increment,ScrubAreaScrubArearenders the ApsaraLabelcomponent internally for drag-to-adjust interaction--rs-*tokens, tabular-nums)