feat: introduce NumberInput#4475
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
packages/components/number-input/src/use-number-input.ts (3)
20-86: Enhance Props interface documentation with examples.The Props interface is well-documented, but adding code examples for key props like
startContentandendContentwould improve developer experience.Add examples like:
/** * Element to be rendered in the left side of the input. + * @example + * ```tsx + * <NumberInput + * startContent={<Icon name="dollar" />} + * value={100} + * /> + * ``` */ startContent?: React.ReactNode;
231-240: Optimize slots memoization dependencies.The
objectToDepsdependency in the useMemo hook's dependency array might cause unnecessary re-renders. Consider flattening the variant props.const slots = useMemo( () => numberInput({ ...variantProps, isInvalid, isClearable, disableAnimation, }), - [objectToDeps(variantProps), isInvalid, isClearable, hasStartContent, disableAnimation], + [ + variantProps.size, + variantProps.color, + // ... other specific variant props + isInvalid, + isClearable, + hasStartContent, + disableAnimation + ], );
491-505: Enhance stepper buttons accessibility.The stepper buttons should have descriptive ARIA labels to improve accessibility for screen readers.
const getStepperIncreaseButtonProps: PropGetter = useCallback( (props = {}) => { return { ...props, type: "button", disabled: originalProps.isDisabled, + "aria-label": "Increase value", "data-slot": "increase-button", className: slots.stepperButton({ class: clsx(classNames?.stepperButton, props?.className), }), ...mergeProps(incrementButtonProps, props), }; }, [slots], ); const getStepperDecreaseButtonProps: PropGetter = useCallback( (props = {}) => { return { type: "button", disabled: originalProps.isDisabled, + "aria-label": "Decrease value", "data-slot": "decrease-button", className: slots.stepperButton({ class: clsx(classNames?.stepperButton, props?.className), }), ...mergeProps(decrementButtonProps, props), }; }, [slots], );Also applies to: 507-520
apps/docs/content/components/number-input/custom-validation.raw.jsx (2)
20-30: Improve validation logic maintainability.Consider extracting validation constants and using early returns for better maintainability.
Here's a suggested improvement:
+const MIN_VALUE = 100; +const MAX_VALUE = 1000; +const SPECIAL_VALUE = 777; validate={(value) => { - if (value < 100) { - return "Number must be greater than 100"; + if (value < MIN_VALUE) { + return `Number must be greater than ${MIN_VALUE}`; } - if (value > 1000) { - return "Number must be less than 1000"; + if (value > MAX_VALUE) { + return `Number must be less than ${MAX_VALUE}`; } - return value === 777 ? "Nice try!" : null; + return value === SPECIAL_VALUE ? "Nice try!" : null; }}
14-40: Enhance form accessibility.Consider adding ARIA attributes and form validation feedback for better accessibility.
Here's a suggested improvement:
-<Form className="w-full max-w-xs" onSubmit={onSubmit}> +<Form + className="w-full max-w-xs" + onSubmit={onSubmit} + aria-label="Number validation form" +> <NumberInput isRequired label="Amount" name="amount" placeholder="Enter a number" + aria-describedby="amount-validation" validate={(value) => { // ... validation logic }} /> {submitted && ( - <div className="text-small text-default-500"> + <div className="text-small text-default-500" role="status"> You submitted: <code>{JSON.stringify(submitted)}</code> </div> )} </Form>apps/docs/content/components/number-input/real-time-validation.raw.jsx (1)
3-54: Optimize performance by memoizing validation logic.The validation logic and errors array are recreated on every render, which can lead to unnecessary re-renders.
Apply this diff to optimize the performance:
export default function App() { const [submitted, setSubmitted] = React.useState(null); const [amount, setAmount] = React.useState(null); - const errors = []; + const validate = React.useCallback((value) => { + const errors = []; + if (!value) { + errors.push("The value must not be empty"); + } + if (value < 100) { + errors.push("The value must be greater than 100"); + } + if (value > 1000) { + errors.push("The value must be less than 1000"); + } + return errors; + }, []); + const errors = React.useMemo(() => validate(amount), [amount, validate]); const onSubmit = (e) => { e.preventDefault(); const data = Object.fromEntries(new FormData(e.currentTarget)); setSubmitted(data); }; - if (!amount) { - errors.push("The value must not be empty"); - } - - if (amount < 100) { - errors.push("The value must be greater than 100"); - } - - if (amount > 1000) { - errors.push("The value must be less than 1000"); - } return ( <Form className="w-full max-w-xs" onSubmit={onSubmit}>packages/core/theme/src/components/number-input.ts (1)
24-824: LGTM! Consider enhancing keyboard navigation.The theme configuration is well-structured with comprehensive support for variants, colors, and RTL layouts. The accessibility features through ARIA attributes are properly implemented.
Consider adding keyboard focus styles for better accessibility:
input: [ "w-full font-normal bg-transparent !outline-none placeholder:text-foreground-500 focus-visible:outline-none", "data-[has-start-content=true]:ps-1.5", "data-[has-end-content=true]:pe-1.5", "autofill:bg-transparent bg-clip-text", + "focus-visible:ring-2 focus-visible:ring-primary-500", ],apps/docs/content/docs/components/number-input.mdx (2)
364-372: Fix preposition usage in prop descriptions.The descriptions for
startContentandendContentprops use incorrect prepositions.Apply this diff to fix the grammar:
- description: "Element to be rendered in the left side of the input.", + description: "Element to be rendered on the left side of the input.", default: "-" }, { attribute: "endContent", type: "ReactNode", - description: "Element to be rendered in the right side of the input.", + description: "Element to be rendered on the right side of the input.", default: "-" },🧰 Tools
🪛 LanguageTool
[grammar] ~365-~365: In this case, the usual preposition with “side” is “on”, not “in”. Did you mean “on the left side”?
Context: ... description: "Element to be rendered in the left side of the input.", default: "-" ...(IN_ON_THE_RIGHT_HAND_SIDE)
[grammar] ~371-~371: In this case, the usual preposition with “side” is “on”, not “in”. Did you mean “on the right side”?
Context: ... description: "Element to be rendered in the right side of the input.", default: "-" ...(IN_ON_THE_RIGHT_HAND_SIDE)
226-233: Enhance accessibility documentation with keyboard navigation details.The accessibility section could benefit from specific keyboard navigation instructions.
Add keyboard navigation instructions:
## Accessibility - Built with a native `<input>` element with `type="number"`. - Visual and ARIA labeling support. - Change, clipboard, composition, selection, and input event support. - Required and invalid states exposed to assistive technology via ARIA. - Support for description, helper text, and error message linked to the input via ARIA. +- Keyboard Navigation: + - Use `Up Arrow` or `Down Arrow` to increment or decrement the value. + - Use `Page Up` or `Page Down` to increment or decrement by a larger step. + - Use `Home` or `End` to set the value to the minimum or maximum.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
apps/docs/config/routes.json(1 hunks)apps/docs/content/components/number-input/custom-validation.raw.jsx(1 hunks)apps/docs/content/components/number-input/real-time-validation.raw.jsx(1 hunks)apps/docs/content/components/number-input/server-validation.raw.jsx(1 hunks)apps/docs/content/docs/components/number-input.mdx(1 hunks)apps/docs/content/docs/guide/forms.mdx(1 hunks)packages/components/number-input/__tests__/number-input.test.tsx(1 hunks)packages/components/number-input/package.json(1 hunks)packages/components/number-input/src/number-input-stepper.tsx(1 hunks)packages/components/number-input/src/use-number-input.ts(1 hunks)packages/core/theme/src/components/number-input.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/components/number-input/src/number-input-stepper.tsx
- apps/docs/content/components/number-input/server-validation.raw.jsx
- packages/components/number-input/package.json
- packages/components/number-input/tests/number-input.test.tsx
- apps/docs/content/docs/guide/forms.mdx
- apps/docs/config/routes.json
🧰 Additional context used
🪛 LanguageTool
apps/docs/content/docs/components/number-input.mdx
[style] ~146-~146: To form a complete sentence, be sure to include a subject.
Context: ...rm.com/). ### With Form NumberInput can be used with a Form component to leve...
(MISSING_IT_THERE)
[grammar] ~365-~365: In this case, the usual preposition with “side” is “on”, not “in”. Did you mean “on the left side”?
Context: ... description: "Element to be rendered in the left side of the input.", default: "-" ...
(IN_ON_THE_RIGHT_HAND_SIDE)
[grammar] ~371-~371: In this case, the usual preposition with “side” is “on”, not “in”. Did you mean “on the right side”?
Context: ... description: "Element to be rendered in the right side of the input.", default: "-" ...
(IN_ON_THE_RIGHT_HAND_SIDE)
🔇 Additional comments (5)
packages/components/number-input/src/use-number-input.ts (2)
155-155: UseNumber.isNaNfor better type safety.Relying on the global
isNaNcan cause unexpected type coercions.
562-562: LGTM!The hook return type is correctly implemented using TypeScript's ReturnType utility.
apps/docs/content/components/number-input/custom-validation.raw.jsx (2)
1-1: LGTM!The imports are correctly structured and include all necessary components.
6-11: Add error handling for form submission.The form submission handler should include error handling and loading state management.
apps/docs/content/docs/components/number-input.mdx (1)
1-148: LGTM! Documentation is comprehensive and well-structured.The documentation provides clear installation instructions, usage examples, and accessibility information.
🧰 Tools
🪛 LanguageTool
[style] ~146-~146: To form a complete sentence, be sure to include a subject.
Context: ...rm.com/). ### With FormNumberInputcan be used with aFormcomponent to leve...(MISSING_IT_THERE)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/components/number-input/src/number-input-stepper.tsx (1)
11-17: 🛠️ Refactor suggestionAdd proper state handling and icon sizing.
The component implementation needs enhancement with:
- Error handling for invalid direction values
- Icon size customization
- Disabled and loading state handling
- Use strict equality (===) instead of loose equality (==)
-const NumberInputStepper = ({direction, ...otherProps}: NumberInputStepperProps) => { +const NumberInputStepper = ({ + direction, + size = "sm", + isDisabled, + isLoading, + ...otherProps +}: NumberInputStepperProps) => { + if (!["up", "down"].includes(direction)) { + console.warn(`Invalid direction: ${direction}. Using "up" as fallback.`); + } + return ( - <Button isIconOnly {...otherProps}> - {direction == "up" ? <ChevronUpIcon /> : <ChevronDownIcon />} + <Button + isIconOnly + isDisabled={isDisabled} + isLoading={isLoading} + size={size} + {...otherProps} + > + {direction === "up" ? ( + <ChevronUpIcon size={size === "sm" ? 16 : 20} /> + ) : ( + <ChevronDownIcon size={size === "sm" ? 16 : 20} /> + )} </Button> ); };
🧹 Nitpick comments (3)
packages/components/number-input/stories/number-input.stories.tsx (3)
3-11: Organize imports for better maintainability.Group imports by external and internal dependencies, and sort them alphabetically within each group.
-import type {ValidationResult} from "@react-types/shared"; - -import React from "react"; -import {Meta} from "@storybook/react"; -import {button} from "@heroui/theme"; -import {Form} from "@heroui/form"; -import {numberInput} from "@heroui/theme"; - -import {NumberInput, NumberInputProps} from "../src"; +// External dependencies +import {Meta} from "@storybook/react"; +import React from "react"; +import type {ValidationResult} from "@react-types/shared"; + +// Internal dependencies +import {button, numberInput} from "@heroui/theme"; +import {Form} from "@heroui/form"; +import {NumberInput, NumberInputProps} from "../src";
59-65: Consider using min-height instead of height for better responsiveness.The decorator uses a fixed height which might cause issues with different screen sizes.
- <div className="flex items-center justify-center w-screen h-screen"> + <div className="flex items-center justify-center w-screen min-h-screen p-4">
417-427: Simplify validation logic using a lookup object.The current validation logic using if statements could be simplified for better maintainability.
- errorMessage: (value: ValidationResult) => { - if (value.validationDetails.rangeOverflow) { - return "Value is too high"; - } - if (value.validationDetails.rangeUnderflow) { - return "Value is too low"; - } - if (value.validationDetails.valueMissing) { - return "Value is required"; - } - }, + errorMessage: (value: ValidationResult) => { + const errorMessages = { + rangeOverflow: "Value is too high", + rangeUnderflow: "Value is too low", + valueMissing: "Value is required" + }; + const firstError = Object.keys(value.validationDetails).find( + (key) => value.validationDetails[key] + ); + return firstError ? errorMessages[firstError] : undefined; + },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
apps/docs/config/routes.json(1 hunks)packages/components/number-input/src/number-input-stepper.tsx(1 hunks)packages/components/number-input/stories/number-input.stories.tsx(1 hunks)packages/core/react/package.json(1 hunks)packages/core/react/src/index.ts(1 hunks)packages/core/theme/src/components/index.ts(1 hunks)packages/utilities/shared-icons/src/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/core/theme/src/components/index.ts
- packages/core/react/src/index.ts
- packages/utilities/shared-icons/src/index.ts
- packages/core/react/package.json
- apps/docs/config/routes.json
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: TypeScript
- GitHub Check: Continuous Release
- GitHub Check: Build
🔇 Additional comments (2)
packages/components/number-input/src/number-input-stepper.tsx (2)
1-9: LGTM! Well-structured type definitions and imports.The type definitions are properly structured with clear separation of concerns. The
NumberInputStepperPropsinterface correctly extendsButtonPropswhile omittingAriaButtonPropsto avoid conflicts.
19-21: LGTM! Proper component metadata and export.The component's display name follows the library's naming convention, which is helpful for debugging. The default export is appropriate for the main component.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/core/theme/src/components/number-input.ts (2)
24-89: Consider adding ARIA attributes for better accessibility.The component's base slots are well-defined, but could benefit from additional accessibility enhancements.
Consider adding these ARIA-related classes to improve accessibility:
input: [ "w-full font-normal bg-transparent !outline-none placeholder:text-foreground-500 focus-visible:outline-none", "data-[has-start-content=true]:ps-1.5", "data-[has-end-content=true]:pe-1.5", "autofill:bg-transparent bg-clip-text", + "aria-invalid:border-danger", + "aria-disabled:opacity-disabled", + "aria-disabled:cursor-not-allowed", ],
283-845: Consider reducing compound variants complexity.The compound variants section is quite extensive and might benefit from being split into smaller, more manageable chunks.
Consider extracting compound variants into separate constants grouped by feature:
const colorVariants = [ // flat & color variants { variant: "flat", color: "primary", class: {/*...*/} }, // ... ]; const sizeVariants = [ // size & labelPlacement variants { labelPlacement: "inside", size: "sm", class: {/*...*/} }, // ... ]; const numberInput = tv({ // ... compoundVariants: [ ...colorVariants, ...sizeVariants, // ... ] });packages/components/number-input/stories/number-input.stories.tsx (1)
405-428: Enhance error handling in WithErrorMessageFunction story.The validation logic could be improved by:
- Adding a default case to handle unexpected validation states
- Providing more descriptive error messages that include the valid range
errorMessage: (value: ValidationResult) => { if (value.validationDetails.rangeOverflow) { - return "Value is too high"; + return "Value must be less than or equal to 100"; } if (value.validationDetails.rangeUnderflow) { - return "Value is too low"; + return "Value must be greater than or equal to 0"; } if (value.validationDetails.valueMissing) { return "Value is required"; } + return "Please enter a valid number between 0 and 100"; },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/components/number-input/stories/number-input.stories.tsx(1 hunks)packages/core/theme/src/components/number-input.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: TypeScript
- GitHub Check: Continuous Release
- GitHub Check: Build
🔇 Additional comments (6)
packages/core/theme/src/components/number-input.ts (4)
1-23: LGTM! Well-documented component with clear usage examples.The component is properly documented with TypeScript JSDoc comments and includes a practical example demonstrating the usage of various slots.
90-273: LGTM! Comprehensive variant system with proper dark mode support.The variants are well-organized and include proper support for different themes, sizes, colors, and dark mode.
275-282: LGTM! Sensible default variants.The default variants provide a good starting point for the component.
848-851: LGTM! Clear and concise exports.The type exports are well-defined and follow TypeScript best practices.
packages/components/number-input/stories/number-input.stories.tsx (2)
1-2: Address accessibility concerns instead of disabling ESLint rules.Rather than disabling accessibility-related ESLint rules, implement proper keyboard interactions and focus management.
173-194: Improve type safety in ServerValidationTemplate.The template lacks proper TypeScript types for the event handler and server errors.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/core/theme/src/components/number-input.ts (4)
50-67: Consider improving the clear button's default state.The
clearButtonslot haspointer-events-noneby default, which might affect usability when the button becomes visible. Consider moving this class to the compound variant forisClearableto ensure it only applies when needed.clearButton: [ "p-2", "-m-2", "z-10", "end-3", "start-auto", - "pointer-events-none", "appearance-none", "outline-none", "select-none", "opacity-0", "hover:!opacity-100", "cursor-pointer", "active:!opacity-70", "rounded-full", // focus ring ...dataFocusVisibleClasses, ],
68-85: Consider using CSS variables for stepper button dimensions.The
stepperButtonslot has hardcoded width and height values. Consider using CSS variables for better maintainability and consistency across different sizes.stepperButton: [ "bg-transparent", "flex", "justify-center", "items-center", "before:absolute", "before:w-8", // the max width that won't block clear button "before:h-8", "before:rounded-full", "after:shadow-small", "after:bg-background", "data-[focused=true]:z-10", - "min-w-5", - "w-5", - "h-5", + "min-w-[theme(spacing.5)]", + "w-[theme(spacing.5)]", + "h-[theme(spacing.5)]", "overflow-visible", "data-[pressed=true]:opacity-70", ],
117-142: Consider using Tailwind's built-in shadow utility for underlined variant.The underlined variant uses a custom shadow implementation. Consider using Tailwind's built-in shadow utility for better maintainability and consistency.
underlined: { inputWrapper: [ "!px-1", "!pb-0", "!gap-0", "relative", "box-border", "border-b-medium", - "shadow-[0_1px_0px_0_rgba(0,0,0,0.05)]", + "shadow-sm", "border-default-200", "!rounded-none", "hover:border-default-300", // ... rest of the classes ], },
323-339: Consider consistent dark mode handling for success and warning colors.The success and warning color variants have inconsistent dark mode text color handling. The success variant uses
dark:text-successwhile the warning variant usesdark:text-warning. Consider using a consistent approach.{ variant: "flat", color: "success", class: { inputWrapper: [ "bg-success-100", - "text-success-600", - "dark:text-success", + "text-success", "placeholder:text-success-600", "dark:placeholder:text-success", "data-[hover=true]:bg-success-50", "group-data-[focus=true]:bg-success-50", ], - input: "placeholder:text-success-600 dark:placeholder:text-success", - label: "text-success-600 dark:text-success", + input: "placeholder:text-success", + label: "text-success", }, }, { variant: "flat", color: "warning", class: { inputWrapper: [ "bg-warning-100", - "text-warning-600", - "dark:text-warning", + "text-warning", "placeholder:text-warning-600", "dark:placeholder:text-warning", "data-[hover=true]:bg-warning-50", "group-data-[focus=true]:bg-warning-50", ], - input: "placeholder:text-warning-600 dark:placeholder:text-warning", - label: "text-warning-600 dark:text-warning", + input: "placeholder:text-warning", + label: "text-warning", }, },Also applies to: 340-356
apps/docs/content/docs/components/number-input.mdx (2)
146-146: Improve sentence structure for clarity.The sentence lacks a subject, which might affect readability.
-`NumberInput` can be used with a `Form` component to leverage form state management. +The `NumberInput` component can be used with a `Form` component to leverage form state management.🧰 Tools
🪛 LanguageTool
[style] ~146-~146: To form a complete sentence, be sure to include a subject.
Context: ...rm.com/). ### With FormNumberInputcan be used with aFormcomponent to leve...(MISSING_IT_THERE)
365-365: Fix preposition usage in element descriptions.The descriptions for
startContentandendContentuse incorrect prepositions.-"Element to be rendered in the left side of the input." +"Element to be rendered on the left side of the input." -"Element to be rendered in the right side of the input." +"Element to be rendered on the right side of the input."Also applies to: 371-371
🧰 Tools
🪛 LanguageTool
[grammar] ~365-~365: In this case, the usual preposition with “side” is “on”, not “in”. Did you mean “on the left side”?
Context: ... description: "Element to be rendered in the left side of the input.", default: "-" ...(IN_ON_THE_RIGHT_HAND_SIDE)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/docs/content/docs/components/number-input.mdx(1 hunks)packages/components/number-input/src/number-input-stepper.tsx(1 hunks)packages/core/theme/src/components/number-input.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/components/number-input/src/number-input-stepper.tsx
🧰 Additional context used
🪛 LanguageTool
apps/docs/content/docs/components/number-input.mdx
[style] ~146-~146: To form a complete sentence, be sure to include a subject.
Context: ...rm.com/). ### With Form NumberInput can be used with a Form component to leve...
(MISSING_IT_THERE)
[grammar] ~365-~365: In this case, the usual preposition with “side” is “on”, not “in”. Did you mean “on the left side”?
Context: ... description: "Element to be rendered in the left side of the input.", default: "-" ...
(IN_ON_THE_RIGHT_HAND_SIDE)
[grammar] ~371-~371: In this case, the usual preposition with “side” is “on”, not “in”. Did you mean “on the right side”?
Context: ... description: "Element to be rendered in the right side of the input.", default: "-" ...
(IN_ON_THE_RIGHT_HAND_SIDE)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: TypeScript
- GitHub Check: Continuous Release
- GitHub Check: Build
🔇 Additional comments (2)
packages/core/theme/src/components/number-input.ts (1)
849-852: LGTM!The type exports are well-defined and provide good type safety for component consumers.
apps/docs/content/docs/components/number-input.mdx (1)
1-30: LGTM!The component description and installation instructions are clear, comprehensive, and well-organized.
2 participants
📝 Description
TODO:
⛳️ Current behavior (updates)
🚀 New behavior
💣 Is this a breaking change (Yes/No):
📝 Additional Information
Summary by CodeRabbit
Here are the updated release notes for the Number Input component:
New Features
NumberInputcomponent for numeric input.NumberInputSteppercomponent for increment and decrement actions.Improvements
Documentation
Performance