fix(autocomplete): return value instead of label in form submission #3375
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: 09bcf2b The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (5)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes in this pull request modify the behavior of the autocomplete component to return the actual value of the selected option instead of its label during form submission. This adjustment addresses issues #3353 and #3343. Additionally, the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 7
🧹 Outside diff range and nitpick comments (6)
.changeset/purple-pillows-beg.md (1)
1-5: LGTM! Minor suggestion for clarity.The changeset correctly indicates a minor version bump for the autocomplete component, which is appropriate for this functional change. The description accurately summarizes the modification and references the relevant issues.
Consider slightly expanding the change description for added clarity:
-return autocomplete value instead of label in form submission (#3353, #3343) +fix(autocomplete): return selected value instead of label in form submission (#3353, #3343)This minor adjustment adds context about the component being modified and clarifies that it's the selected value being returned.
packages/components/autocomplete/package.json (1)
58-58: Approved: Addition of @react-aria/form dependencyThe addition of the "@react-aria/form" dependency is appropriate for enhancing the form handling capabilities of the Autocomplete component. This change aligns well with the PR objectives to modify the component's behavior during form submissions.
Consider using a caret (^) version range instead of an exact version to allow for compatible updates:
- "@react-aria/form": "3.0.5", + "@react-aria/form": "^3.0.5",This change would allow for minor version updates that include bug fixes and non-breaking changes, potentially improving the package's maintenance in the long run.
packages/components/autocomplete/stories/autocomplete.stories.tsx (1)
767-787: LGTM! The implementation addresses the PR objectives.The
WithFormTemplatefunction correctly demonstrates how the Autocomplete component can be used within a form, addressing the issue of returning the value instead of the label during form submission. A few suggestions for improvement:
- Consider adding TypeScript types for the event parameter in the
handleSubmitfunction.- The
alertandconsole.logare fine for demonstration purposes, but it might be worth adding a comment to indicate that these are for demo purposes only.Here's a suggested improvement for the
handleSubmitfunction:- const handleSubmit = (e) => { + const handleSubmit = (e: React.FormEvent<HTMLFormElement>) => { e.preventDefault(); const name = e.target.animal.value; - // eslint-disable-next-line no-console - console.log(name); - alert("Submitted value: " + name); + // These logs are for demonstration purposes only + console.log("Submitted value:", name); + alert(`Submitted value: ${name}`); };packages/components/autocomplete/src/hidden-input.tsx (2)
9-23: Unusedlabelprop inAriaHiddenInputPropsThe
labelprop is defined inAriaHiddenInputPropsbut is not used in theuseHiddenInputhook or theHiddenInputcomponent. This may cause confusion or indicate leftover code.Consider removing the
labelprop if it's not necessary:export interface AriaHiddenInputProps { /** * Describes the type of autocomplete functionality the input should provide if any. * See [MDN](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input#htmlattrdefautocomplete). */ autoComplete?: string; - /** The text label for the input. */ - label?: ReactNode; /** HTML form input name. */ name?: string; /** Sets the disabled state of the input. */ isDisabled?: boolean; /** Whether the input is required. */ isRequired?: boolean; }Alternatively, if
labelis intended for future use, please ensure it's utilized appropriately.
41-85: Consider adding unit tests foruseHiddenInputandHiddenInputAdding unit tests for these components will help ensure their reliability and catch potential issues early.
Would you like assistance in setting up unit tests for these components or opening a GitHub issue to track this task?
packages/components/autocomplete/src/use-autocomplete.ts (1)
553-575: Optimize dependency array inuseCallbackhookIn the
getHiddenInputPropsfunction, some dependencies in theuseCallbackhook's dependency array may be unnecessary or could cause unnecessary re-creations of the function.Review the dependencies and consider simplifying them:
useCallback( (props = {}) => ({ state, inputRef, hiddenInputRef, name: originalProps?.name, isRequired: originalProps?.isRequired, autoComplete: originalProps?.autoComplete, isDisabled: originalProps?.isDisabled, onChange, ...props, }), [ state, originalProps?.name, originalProps?.autoComplete, originalProps?.isDisabled, originalProps?.isRequired, - inputRef, - hiddenInputRef, + inputRef.current, + hiddenInputRef.current, ], );Alternatively, if
inputRefandhiddenInputRefare stable (do not change across renders), they can be omitted from the dependency array.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
- .changeset/purple-pillows-beg.md (1 hunks)
- packages/components/autocomplete/tests/autocomplete.test.tsx (3 hunks)
- packages/components/autocomplete/package.json (1 hunks)
- packages/components/autocomplete/src/autocomplete.tsx (3 hunks)
- packages/components/autocomplete/src/hidden-input.tsx (1 hunks)
- packages/components/autocomplete/src/use-autocomplete.ts (9 hunks)
- packages/components/autocomplete/stories/autocomplete.stories.tsx (2 hunks)
🧰 Additional context used
🪛 Biome
packages/components/autocomplete/src/use-autocomplete.ts
[error] 464-464: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (12)
packages/components/autocomplete/src/autocomplete.tsx (3)
12-12: LGTM: New import for HiddenInputThe import for HiddenInput is correctly placed and necessary for the new functionality being added to the Autocomplete component.
33-33: LGTM: Addition of getHiddenInputPropsThe inclusion of
getHiddenInputPropsin the destructured return fromuseAutocompleteis consistent with the PR objectives and follows the existing naming conventions. This change enables the component to manage hidden input properties, which is crucial for returning the correct value during form submissions.
48-48: LGTM: Addition of HiddenInput componentThe inclusion of the
HiddenInputcomponent effectively addresses the PR objectives by providing a mechanism to store and submit the actual value instead of the label. Its placement and usage ofgetHiddenInputPropsare appropriate.To ensure this change works as intended, please run the following verification script:
This script will help verify the correct implementation and usage of the HiddenInput component. Please review the results to ensure everything is in order.
packages/components/autocomplete/stories/autocomplete.stories.tsx (2)
1029-1035: LGTM! The export is correctly implemented.The
WithFormexport is properly added, following the established pattern for other examples in this file. It correctly uses theWithFormTemplaterender function and includes thedefaultProps.
Line range hint
767-1035: Summary: The changes effectively address the PR objectives.The addition of the
WithFormTemplateand its corresponding exportWithFormsuccessfully demonstrates the correct usage of the Autocomplete component within a form. These changes address the issue where the Autocomplete component was returning the label instead of the value during form submission.Key improvements:
- The Autocomplete component now uses a
nameprop, allowing it to be included in form data.- The
handleSubmitfunction correctly retrieves the value usinge.target.animal.value.- The AutocompleteItems use different keys and labels, effectively testing if the correct value (key) is submitted.
These changes will help users understand how to properly implement form submission with the Autocomplete component, resolving the issues mentioned in #3353 and #3343.
packages/components/autocomplete/src/hidden-input.tsx (6)
1-6: Approved: Import statements are appropriateThe import statements correctly bring in all required dependencies.
7-8: Approved: ImportinginputDataThe
inputDatais properly imported from"./use-autocomplete".
25-31: Approved: Combining input props correctlyThe types
NativeHTMLInputPropsandCombinedAriaInputPropsare correctly defined to merge native input attributes with custom props.
32-39: Approved: Definition ofHiddenInputPropsThe
HiddenInputPropsinterface appropriately extendsCombinedAriaInputPropsand includes additional properties needed for the component.
73-77: Verify thatonChangehandler updates state correctlyIn the
onChangehandler for the hidden input,state.setSelectedKey(e.target.value)updates the selected key in the state. Ensure that this correctly synchronizes with the visible input and that theonChangeprop behaves as expected.
81-85: Approved:HiddenInputcomponent implementationThe
HiddenInputcomponent correctly utilizes theuseHiddenInputhook and renders an<input>element with the appropriate properties.packages/components/autocomplete/__tests__/autocomplete.test.tsx (1)
867-888: Test case effectively verifies form value correctnessThe newly added test case robustly checks that the form values are correctly captured after user interactions, ensuring the
Autocompletecomponent integrates well with React Hook Form.
packages/components/autocomplete/__tests__/autocomplete.test.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
packages/components/autocomplete/src/use-autocomplete.ts (2)
118-125: LGTM: NewInputDatainterface andinputDataWeakMap addedThe addition of the
InputDatainterface andinputDataWeakMap is a good approach for managing input-related data. The use of a WeakMap is efficient and allows for garbage collection of unusedComboBoxStateinstances.However, it would be helpful to add a comment explaining the purpose of the
inputDataWeakMap for better code maintainability.Consider adding a comment above the
inputDatadeclaration, like this:// WeakMap to associate ComboBoxState instances with their corresponding InputData // This allows for efficient storage and retrieval of input-related data export const inputData = new WeakMap<ComboBoxState<any>, InputData>();
Line range hint
181-224: LGTM with suggestion: Implementation ofonSelectionChangeThe implementation of
onSelectionChangein theuseComboBoxStateconfiguration is good. It correctly handles selection changes and updates the form value, addressing the main objective of returning the actual value instead of the label.However, there's a potential issue when accessing
hiddenInputRef.current.Add a null check before accessing
hiddenInputRef.currentto prevent potential runtime errors:onSelectionChange: (keys) => { onSelectionChange?.(keys); if (onChange && typeof onChange === "function") { onChange({ target: { name: hiddenInputRef?.current?.name, value: keys, }, } as React.ChangeEvent<HTMLInputElement>); } },Consider refactoring to:
onSelectionChange: (keys) => { onSelectionChange?.(keys); if (onChange && typeof onChange === "function") { const name = hiddenInputRef?.current?.name; onChange({ target: { name, value: keys, }, } as React.ChangeEvent<HTMLInputElement>); } },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- packages/components/autocomplete/src/use-autocomplete.ts (9 hunks)
🧰 Additional context used
🪛 Biome
packages/components/autocomplete/src/use-autocomplete.ts
[error] 464-464: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (3)
packages/components/autocomplete/src/use-autocomplete.ts (3)
113-117: LGTM: NewonSelectionChangeprop addedThe addition of the
onSelectionChangeprop is a good improvement. It allows users to handle selection changes, which is crucial for the new functionality of returning the actual value instead of the label.
128-131: LGTM: UpdatedUseAutocompletePropstypeThe update to the
UseAutocompleteProps<T>type, excludingonSelectionChangefrom theOmit<InputProps, ...>part, is correct. This change ensures that the newly addedonSelectionChangeprop is properly typed and doesn't conflict with any similarly named prop fromInputProps.
Line range hint
553-609: LGTM: Addition ofgetHiddenInputPropsand related changesThe addition of the
getHiddenInputPropsfunction and the related changes are well-implemented:
- The
getHiddenInputPropsfunction is correctly memoized to optimize performance.- The
inputDataWeakMap is properly populated with the necessary input-related data.- The addition of
getHiddenInputPropsto the hook's return value allows consumers to easily apply the hidden input properties.These changes effectively support the new functionality of using a hidden input to store the actual value, which is a key objective of this PR.
|
is there any progress on this issue? please merge so i can use the autocomplete component |
|
Thanks. really appreciate it 🙌👌 |
|
@wingkwong @jrgarciadev For example, if the key is a sent as the value, the Custom value feature will send the key instead of the user's input. This would not be useful. |
|
let's have an internal discussion. |
|
As mentioned in this comment, autocomplete is not a select, so the label is submitted. |
📝 Description
Similar to Select, use a hidden input to hold the actual value.
⛳️ Current behavior (updates)
The value in Input is actually label. In form submission, users need the value instead of the label.
🚀 New behavior
💣 Is this a breaking change (Yes/No):
Yes. Existing form values will be different after this change.
📝 Additional Information
Summary by CodeRabbit
New Features
WithFormTemplatefor submitting selected values, displaying alerts upon submission.Bug Fixes
Tests
Autocompletecomponent, ensuring accurate form value retrieval and validation.Documentation