-
Notifications
You must be signed in to change notification settings - Fork 858
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
Autocomplete was implemented #763
base: master
Are you sure you want to change the base?
Autocomplete was implemented #763
Conversation
NazariiAlieksieiev
commented
Oct 17, 2024
- DEMO
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.
Great job on the implementation! 🎉 While there are a few minor naming convention issues and a potential performance consideration, they don't critically impact the functionality. Keep up the good work and consider the feedback for future improvements! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
</div> | ||
<DropdownMenu | ||
people={people} | ||
onSelectedPerson={setSelectedPerson} |
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.
The prop 'onSelectedPerson' does not follow the recommended naming conventions for event handler functions in React. According to the checklist, it should be named using the 'handle' prefix, such as 'handleSelectedPerson'.
); | ||
|
||
const handleInput = (event: React.ChangeEvent<HTMLInputElement>) => { | ||
applyQuery(event.target.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.
The filter should not be called if the user enters spaces only. Consider trimming the input value and checking if it is not empty before calling applyQuery.
style={{ cursor: 'pointer' }} | ||
className="dropdown-item" | ||
data-cy="suggestion-item" | ||
key={person.slug} |
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.
Generating keys during render can lead to performance issues and unexpected behavior. Ensure that the key is stable and unique for each item, and not generated dynamically during render.
[], | ||
); | ||
|
||
const handleInput = (event: React.ChangeEvent<HTMLInputElement>) => { |
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.
Consider following the naming conventions for event handler functions in React. For example, prefix the function name with 'handle' or 'on'.