-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix: Allow selectedKey=null
to clear value in HiddenSelect
#8330
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1715,6 +1715,7 @@ describe('Picker', function () { | |
expect(options.length).toBe(60); | ||
options.forEach((option, index) => index > 0 && expect(option).toHaveTextContent(states[index - 1].name)); | ||
|
||
fireEvent.input(hiddenSelect, {target: {value: 'CA'}}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just double checking, is this just testing that onSelectionChange isn't called twice? asking because it seems it was passing before the changes in HIddenSelect There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah |
||
fireEvent.change(hiddenSelect, {target: {value: 'CA'}}); | ||
expect(onSelectionChange).toHaveBeenCalledTimes(1); | ||
expect(onSelectionChange).toHaveBeenLastCalledWith('CA'); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was changed previously in #7670 to fix a different issue. @snowystinger do you remember what was going on there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that was to fix the autocomplete. If I recall correctly, it viewed the
''
as a controlled value for the field and wouldn't autofill as a result, React would just overwrite it with the empty string again as there was no onChange event. There is apparently an onInput change for it though, so we can use that to update with the value for auto fill.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, I missed the onInput. Does having both cause setSelectedKey to run twice then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good question, i doubt any of our tests check that, so will need to verify