-
Notifications
You must be signed in to change notification settings - Fork 553
feat(autocomplete): implement recent searches #6747
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: master
Are you sure you want to change the base?
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit b0c9428:
|
0509ac1 to
44940ce
Compare
0a8c374 to
efae04b
Compare
efae04b to
8f7e4a4
Compare
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.
looks good. I have a couple questions around interop
|
|
||
| const indicesForPropGetters = [...indices]; | ||
| const indicesConfigForPropGetters = [...indicesConfig]; | ||
| if (showRecent) { |
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 implies we're saving recent searches even if showRecent is false. Should maybe the whole storage be conditional based on showRecent?
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.
Can't call hooks conditionally but I could return a storage with empty function or something yeah
| } | ||
|
|
||
| const LOCAL_STORAGE_KEY_TEST = 'test-localstorage-support'; | ||
| const LOCAL_STORAGE_KEY = 'autocomplete-recent-searches'; |
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.
Is there some way where we link the autocomplete setup with the recent searches that are being presented? Do we do that in autocomplete.js?
What if there's multiple autocomplete instances in multiple places in the app?
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 think we could but the original lib doesn't, even though it does have code to handle multiple autocomplete components on a page (which I don't think anyone needs really)
Summary
FX-3504
Result