-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat: Made searchForm into generic combobox with two variations (for search & select) #2659
Conversation
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 have a few thoughts on this but haven't been able to read through and grok all the code yet, sorry!
I'm running out of time today, so I'll try and look at this later tonight or tomorrow morning at the very latest
Coverage of commit
|
<GrouplessAutocompleteFromArray | ||
controlName="" | ||
fallbackOption={autocompleteOption( | ||
"No matching routes", |
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.
nit(non-blocking): we chatted about this in a huddle, but it may be beneficial if we rename this from Combobox
to something about how this is a Map search OR route select at this moment.
I'll post the combobox refactor ticket we talked about on this comment thread for posterity once it's created.
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 that's right, I remember discussing, but I'm not sure what options were preferred. Rename from Combobox
to InputBoxWithDropdown
or DropdownSearchOrSelect
or something of the sort?
Co-authored-by: Kayla Firestack <firestack@users.noreply.github.com>
…into hp/combobox-refactor
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.
Feeling good that all feedback is either addressed or being addressed, left one more nit if you want to do it, otherwise I don't think I need to rereview 👍🏻
Coverage of commit
|
Coverage of commit
|
There is 1 failing test, but I can't test it without the vpn! Will do tomorrow at the office