- Notifications
You must be signed in to change notification settings - Fork928
chore: Add user autocomplete#4210
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| const query = value ? value.email : "" | ||
| sendSearch("SEARCH", { query }) | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []) |
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.
We have some users in v1 with 1000 ~ 2,500 users so loading them at the beginning can be heavy. I think we could only load this when the user start to type in the search box + debouce(to avoid extra loads during typing). What do you think?
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.
@BrunoQuaresma good point! Your solution sounds right. So we can get this out there (@kylecarbs wants to use it) how about we do something like:
useEffect(() => { if (value) { sendSearch("SEARCH", { query: value.email }) } // eslint-disable-next-line react-hooks/exhaustive-deps }, [])for now?
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.
Sounds good!
| const query = selectedValue ? selectedValue.email : "" | ||
| sendSearch("SEARCH", { query }) | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [selectedValue]) |
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 is already done in thehandleFilterChange. I don't think we need the useEffect here 🤔
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.
Hmm, this is what I'm trying to solve with thisuseEffect:
Is there a better way?
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.
Ahhh I see. So when you clear the input, you want to clear the results as well right? If it is, I would add a new event to the search machine calledCLEAR_RESULTS that sets the results to undefined and do this logic in the onChange handler like:
....// This happens when the user clears the results with the "x" buttonif(option === null) { send("CLEAR_RESULTS")}Unfortunately, the MUI Autocomplete does not have anonClear 😔 that would be a better place. I like to put logic on event handlers because I think they are better to follow when they are called than useEffect but it is up to you. If you think this is not necessary feel free to merge it.
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.
That worked well! Thanks!
This is the component we are using for the "groups" feature. Maybe it needs to be more polished and add some stories/tests but since the stories don't work well for components with "toggle states and animations" I think it is ok. Also, tests for this kind of component can be challenging and I'm not sure if it is worth adding.