fix(select): remove input from hidden-select #4427
Conversation
🦋 Changeset detectedLatest commit: 5b5796c The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request focuses on improving the validation behavior of the Select component in the NextUI library. The changes address issues with native validation, error handling, and component state management. The modifications span across multiple files, including test suites, documentation, and core component logic, with the primary goal of ensuring consistent validation behavior across different form components. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 0
🧹 Nitpick comments (6)
packages/components/select/__tests__/select.test.tsx (5)
1183-1230: Add test coverage for unselected edge case
This test scenario looks good and ensures that selecting a different option resolves the validation error. As an optional improvement, consider adding a test for the unselected state (e.g., no items selected) to verify that the form is valid when it's not required or to confirm that an appropriate validation message appears if it is required.
1231-1300: Enhance server validation coverage
The coverage for setting and clearing error states on user interaction is correct. You may want to expand this further by adding a test for reverting to the previously selected item to ensure that the error consistently clears or persists as expected in all transitions.
Line range hint
1303-1356: Consider testing default value scenario
When using isRequired with native validation, the coverage is thorough. However, you could add a test covering the scenario where a default value is already set on the required select, confirming that the form is immediately valid without user interaction.
1360-1413: Validate function test covers key workflow
This test properly verifies native validation with a custom validate function. To strengthen reliability, consider adding an additional assertion for switching back to an invalid selection (e.g., "penguin") after choosing a valid value—ensuring that the error reappears.
1414-1483: Comprehensive server validation scenario
The test successfully demonstrates changing server error states under native validation. As a minor enhancement, adding a test to ensure that repeated submissions aggregate or replace errors properly could further validate robust error handling.packages/components/select/src/hidden-select.tsx (1)
122-122: Avoiding large hidden The threshold of 300 items for deciding between a hidden select and a hidden input is a practical performance optimization. Consider making this threshold a configurable prop for flexibility in different use cases. 📜 Review details Configuration used: .coderabbit.yaml Review profile: CHILL Plan: Pro 📥 Commits Reviewing files that changed from the base of the PR and between 5494fa2 and 5b5796c. 📒 Files selected for processing (6) .changeset/lemon-cheetahs-grow.md (1 hunks) apps/docs/content/docs/components/select.mdx (0 hunks) packages/components/select/__tests__/select.test.tsx (3 hunks) packages/components/select/src/hidden-select.tsx (2 hunks) packages/components/select/src/use-select.ts (1 hunks) packages/hooks/use-aria-multiselect/src/use-multiselect-state.ts (0 hunks) 💤 Files with no reviewable changes (2) packages/hooks/use-aria-multiselect/src/use-multiselect-state.ts apps/docs/content/docs/components/select.mdx 🔇 Additional comments (4) .changeset/lemon-cheetahs-grow.md (1) 1-7: Informative and succinct changeset These changeset notes clearly document the introduced patches. Everything appears fine, and the descriptions are appropriately concise. No additional action needed. packages/components/select/src/hidden-select.tsx (2) 92-92: Hidden input style Setting "display: none" ensures the input is truly hidden. Be sure to confirm that screen readers do not inadvertently pick it up, but since the style is hidden alongside additional ARIA attributes, it should remain accessible only to form submission logic. 95-99: Validate ARIA attributes The ARIA attributes (aria-invalid, aria-required) and required prop are conditionally applied based on the validationBehavior. This is a reliable way to handle two different validation modes. No issues found here—just ensure any upstream or downstream code also differentiates between "aria" and "native" modes as needed. packages/components/select/src/use-select.ts (1) 304-304: Timely validation submission Calling “state.commitValidation();” upon selection change ensures that errors are recalculated and displayed promptly. This is a valuable addition, especially for real-time feedback.
Closes #3913
Alternative to #4281
📝 Description
hidden-selecthas a hidden input that's causing the browserrequirederror popup to appear instead of NextUI'shidden-selectis adapted from an old version ofHiddenSelectfrom thereact-spectrumrepovalidationBehavior=native, which causes error messages to not clearstate.commitValidationwhen value changes⛳️ Current behavior (updates)
when
isRequiredandvalidationBehavior=native, select is showing browser error popupa1.mp4
when
validationBehavior=native, error does not clear on changea2.mp4
🚀 New behavior
when
isRequiredandvalidationBehavior=native, select shows the correct error messageb1.mp4
when
validationBehavior=native, error clears on changeb2.mp4
💣 Is this a breaking change (Yes/No):
No
📝 Additional Information
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests