fix(checkbox): pass form id to input tag #5518
Conversation
🦋 Changeset detectedLatest commit: f5326da The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@ksenijakivojenko is attempting to deploy a commit to the HeroUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes update the Changes
Sequence Diagram(s)sequenceDiagram
participant ConsumerApp
participant Checkbox
participant Form
ConsumerApp->>Checkbox: Render <Checkbox form="form-123" name="terms" />
Checkbox->>Checkbox: Pass form prop to input element
ConsumerApp->>Form: Render <Form id="form-123" />
Note over Checkbox,Form: Checkbox input is outside Form but linked via form="form-123"
ConsumerApp->>Form: Submit Form
Form->>Form: Collect form data (includes Checkbox input)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
.changeset/tasty-glasses-marry.md (1)
1-7: Remove @heroui/form from the changesetWe ran a check against
packages/components/formand found no modified files in this PR:
git status --porcelainreturned no changes inpackages/components/formSince there are no updates to the form package, please remove it from your changeset (or add the corresponding form-related changes if they were intended).
• File:
.changeset/tasty-glasses-marry.md
• Remove this line:-"@heroui/form": minor
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/tasty-glasses-marry.md(1 hunks)packages/components/checkbox/src/use-checkbox.ts(2 hunks)packages/components/checkbox/stories/checkbox.stories.tsx(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/components/checkbox/stories/checkbox.stories.tsx (4)
packages/components/checkbox/src/index.ts (2)
CheckboxProps(9-9)Checkbox(17-17)apps/docs/content/components/input/built-in-validation.raw.jsx (2)
submitted(4-4)onSubmit(6-11)packages/components/form/src/form.tsx (1)
Form(9-15)packages/core/theme/src/components/button.ts (1)
button(475-475)
🔇 Additional comments (3)
packages/components/checkbox/src/use-checkbox.ts (1)
90-90: LGTM: Form prop extraction is correct.The
formprop is properly extracted from the component props, enabling the HTML form attribute functionality for checkboxes.packages/components/checkbox/stories/checkbox.stories.tsx (2)
213-261: Story template demonstrates the new feature effectively.The
SeparateFromFormTemplatecorrectly showcases the new form attribute functionality by rendering a checkbox outside the form but linking it via theformprop. The implementation follows established patterns for form handling and state management.
367-373: Story export is properly configured.The
SeparateFromFormstory export correctly references the new template and uses appropriate default props.
| @@ -313,6 +314,7 @@ export function useCheckbox(props: UseCheckboxProps = {}) { | |||
| ...mergeProps(inputProps, focusProps), | |||
| className: slots.hiddenInput({class: classNames?.hiddenInput}), | |||
| onChange: chain(inputProps.onChange, handleCheckboxChange), | |||
| form: form, | |||
There was a problem hiding this comment.
Fix missing dependency in useMemo hook.
The form prop should be included in the dependency array for the getInputProps callback to ensure proper re-renders when the form value changes.
const getInputProps: PropGetter = useCallback(() => {
return {
ref: mergeRefs(inputRef, ref),
...mergeProps(inputProps, focusProps),
className: slots.hiddenInput({class: classNames?.hiddenInput}),
onChange: chain(inputProps.onChange, handleCheckboxChange),
form: form,
};
- }, [inputProps, focusProps, handleCheckboxChange, classNames?.hiddenInput]);
+ }, [inputProps, focusProps, handleCheckboxChange, classNames?.hiddenInput, form]);📝 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.
| form: form, | |
| const getInputProps: PropGetter = useCallback(() => { | |
| return { | |
| ref: mergeRefs(inputRef, ref), | |
| ...mergeProps(inputProps, focusProps), | |
| className: slots.hiddenInput({class: classNames?.hiddenInput}), | |
| onChange: chain(inputProps.onChange, handleCheckboxChange), | |
| form: form, | |
| }; | |
| }, [inputProps, focusProps, handleCheckboxChange, classNames?.hiddenInput, form]); |
🤖 Prompt for AI Agents
In packages/components/checkbox/src/use-checkbox.ts at line 317, the useMemo
hook for the getInputProps callback is missing the form prop in its dependency
array. Add form to the dependency array of the useMemo hook to ensure the
callback updates correctly when the form value changes.
| e.preventDefault(); | ||
| const data = Object.fromEntries(new FormData(e.currentTarget)); | ||
|
|
||
| if (data.terms == "true") { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use strict equality operator.
Use strict equality (===) instead of loose equality (==) for better type safety and clarity.
- if (data.terms == "true") {
+ if (data.terms === "true") {📝 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.
| if (data.terms == "true") { | |
| if (data.terms === "true") { |
🤖 Prompt for AI Agents
In packages/components/checkbox/stories/checkbox.stories.tsx at line 223,
replace the loose equality operator (==) with the strict equality operator (===)
in the condition checking if data.terms equals "true" to ensure type-safe
comparison.
| label: "text-small", | ||
| }} | ||
| form="heroui-form" | ||
| name="checkbox" |
There was a problem hiding this comment.
Fix checkbox name mismatch.
The checkbox has name="checkbox" but the form submission logic checks for data.terms. This will cause the checkbox value to never be found, making the story non-functional.
- name="checkbox"
+ name="terms"📝 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.
| name="checkbox" | |
| name="terms" |
🤖 Prompt for AI Agents
In packages/components/checkbox/stories/checkbox.stories.tsx at line 240, the
checkbox input has the name attribute set to "checkbox" while the form
submission logic expects the name "terms". To fix this, change the checkbox's
name attribute from "checkbox" to "terms" so that the form data correctly
captures the checkbox value during submission.
Closes
📝 Description
There already is similar request present #4979 but it looks abandoned.
In HTML it is possible to add form id to inputs that are not children of this form and they are being picked up for this form.
⛳️ Current behavior (updates)
Adding form currently works for HeroUI Input component, but not for Checkbox. For Checkbox form parameter is being added to label tag, which will not make it work.
🚀 New behavior
Form parameter is being passed to input tag, making it possible to use checkboxes outside of form.
💣 Is this a breaking change (Yes/No):
No.
Summary by CodeRabbit
New Features
Documentation