-
Notifications
You must be signed in to change notification settings - Fork 32
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
fix: ComboBox show all items on open #2328
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2328 +/- ##
==========================================
- Coverage 46.71% 46.69% -0.02%
==========================================
Files 704 704
Lines 39010 39045 +35
Branches 9747 9756 +9
==========================================
+ Hits 18223 18234 +11
- Misses 20776 20800 +24
Partials 11 11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -10,7 +10,7 @@ import type { NormalizedItem } from '../utils'; | |||
import { type PickerPropsT, usePickerProps } from '../picker'; | |||
|
|||
export type ComboBoxProps = PickerPropsT<SpectrumComboBoxProps<NormalizedItem>>; | |||
|
|||
export { type MenuTriggerAction } from '@react-types/combobox'; |
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.
Aside - not our naming, but I was worried this name would conflict with something from the @react-types/menu
package. But there they have MenuTriggerType
.
I don't know why they didn't call this ComboBoxTriggerType
or something instead, but whatever.
// input. | ||
else { | ||
onSearchTextChange(''); | ||
inputValueRef.current = value; |
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.
Shouldn't this be set in all cases? Everything else looks good.
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.
I'm on the fence on this one. My reasoning for only putting it here was that the only consumer should be the menuTrigger === 'input'
case in onOpenChange
which should only follow this else
case. I was trying to conceptually couple them in an attempt to make the code easier to reason about. That said, there's probably not any harm in setting it for all cases, and there could be an edge case that needs it that I'm unaware of. In general, reasoning about the relationship of these 2 functions is a bit of a pain.
DH-18088 & DH-18087: ComboBox now clears search filter when opening the ComboBox unless it is triggered by user input.
Testing
menu_trigger="focus"
Here's a script with a few different configurations