fix(select): prevent default browser error UI from appearing#4281
fix(select): prevent default browser error UI from appearing#4281chirokas wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: f02168d 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 ↗︎
|
|
@chirokas is attempting to deploy a commit to the NextUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis pull request introduces updates to the Changes
Possibly related PRs
Suggested labels
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: 1
🧹 Outside diff range and nitpick comments (2)
packages/components/select/__tests__/select.test.tsx (1)
1473-1481: Useconstinstead ofletfor variables that are not reassignedIn line 1473,
let {getByTestId} = render(...)should be declared withconstsincegetByTestIdis not reassigned within the scope.Apply the following diff to improve code consistency:
- let {getByTestId} = render( + const {getByTestId} = render(packages/components/select/src/hidden-select.tsx (1)
Line range hint
106-122: Incorrect handling of multiple selections in<select>elementWhen
selectionModeis"multiple", setting thevalueprop to an array ([...state.selectedKeys].map((k) => String(k))) is not standard for native HTML<select>elements. Thevalueattribute expects a string, and for multiple selections, each<option>should have aselectedattribute if it is selected.Additionally, in the
onChangehandler,e.target.valuereturns a string for single selections. For multiple selections, you need to usee.target.selectedOptionsto retrieve all selected values.Please update the code to correctly handle multiple selections:
Instead of setting
valueinselectProps, add theselectedattribute to each<option>element based on whether itsvalueis instate.selectedKeys:<option key={item.key} value={item.key} selected={state.selectedKeys.has(item.key)} > {item.textValue} </option>Modify the
onChangehandler to handle multiple selections:- onChange: (e: React.ChangeEvent<HTMLSelectElement>) => { - state.setSelectedKeys(e.target.value); - onChange?.(e); - }, + onChange: (e: React.ChangeEvent<HTMLSelectElement>) => { + if (selectionMode === "multiple") { + const selectedValues = Array.from(e.target.selectedOptions).map((option) => option.value); + state.setSelectedKeys(new Set(selectedValues)); + } else { + state.setSelectedKeys(new Set([e.target.value])); + } + onChange?.(e); + },This ensures that the hidden
<select>element accurately reflects the selected values and that the form submission works correctly with multiple selections.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
.changeset/quick-buses-kick.md(1 hunks)packages/components/select/__tests__/select.test.tsx(5 hunks)packages/components/select/src/hidden-select.tsx(1 hunks)packages/hooks/use-aria-multiselect/src/use-multiselect-state.ts(1 hunks)
🔇 Additional comments (3)
packages/components/select/__tests__/select.test.tsx (2)
1182-1355: Tests for validationBehavior="native" are comprehensive and well-implemented
The test cases effectively cover scenarios for:
- Required field validation (
isRequired) - Custom validation functions (
validate) - Server-side validation handling
- Clearing validation state on form reset
These tests ensure that the Select component behaves correctly under native validation behavior.
Line range hint 1401-1493: Tests for validationBehavior="aria" are thorough and correctly handle validation states
The test cases appropriately handle:
- Required field validation with custom server errors
- Custom validation functions with ARIA validation behavior
- Server-side validation handling and clearing of validation messages upon user action
The implementation ensures that the Select component provides accessible validation feedback when using ARIA validation behaviors.
.changeset/quick-buses-kick.md (1)
1-6: Changeset is correctly formatted and includes appropriate summary
The changeset properly specifies patch updates for @nextui-org/select and @nextui-org/use-aria-multiselect. The summary message clearly states the purpose of the changes, addressing issue #3913.
| triggerState.close(); | ||
| } | ||
|
|
||
| validationState.commitValidation(); |
There was a problem hiding this comment.
Potential reference error: validationState is used before it is defined
The call to validationState.commitValidation() inside onSelectionChange may cause a runtime error because validationState is declared after listState. Since validationState is not yet initialized when onSelectionChange is created, it will be undefined at the time of the function's execution.
To resolve this issue, consider initializing validationState before listState, or restructure the code so that validationState is accessible within onSelectionChange without causing a reference error.
wingkwong
changed the title
Peterl561
left a comment
Peterl561
left a comment
There was a problem hiding this comment.
Please clean up the tests a little, goes a long way in improving maintainability.
| const select = getByTestId("select"); | ||
| const input = document.querySelector<HTMLSelectElement>("[name=select]"); |
There was a problem hiding this comment.
Select is a <button> and input is a <select>, please consider renaming the variables in the these tests
| const input = document.querySelector<HTMLSelectElement>("[name=select]"); | ||
|
|
||
| expect(input).toHaveAttribute("required"); | ||
| expect(input?.validity.valid).toBe(false); |
There was a problem hiding this comment.
Would be good to check validity.valueMissing too
| expect(select).not.toHaveAttribute("aria-describedby"); | ||
|
|
||
| await user.click(getByTestId("submit")); | ||
| expect(select).toHaveAttribute("aria-describedby"); |
There was a problem hiding this comment.
Should check for aria-invalid on input when a change is expected since input is the actual <select> element and select is just the trigger button
| expect(select).not.toHaveAttribute("aria-describedby"); | ||
|
|
||
| await user.click(getByTestId("reset")); | ||
| expect(select).not.toHaveAttribute("aria-describedby"); |
There was a problem hiding this comment.
This doesn't actually tell us that the reset worked does it
| @@ -99,19 +99,16 @@ export function useHiddenSelect<T>( | |||
| ["data-a11y-ignore"]: "aria-hidden-focus", | |||
| }, | |||
| inputProps: { | |||
There was a problem hiding this comment.
Could probably delete the hidden input element, see adobe/react-spectrum#7200
|
Closing - Handled in #4427 |
3 participants
Closes #3913
Original: #3984
📝 Description
See #3913 (comment)
⛳️ Current behavior (updates)
🚀 New behavior
💣 Is this a breaking change (Yes/No):
📝 Additional Information
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores