solution #758
solution #758
Conversation
src/components/Dropdown/Dropdown.tsx
Outdated
| const handleInputChange = (event: React.ChangeEvent<HTMLInputElement>) => { | ||
| setQuery(event.target.value.trimStart()); | ||
| applyQuery(event.target.value); | ||
| onSelect(null); | ||
| }; |
There was a problem hiding this comment.
The handleInputChange function is not following the recommended naming conventions for event handlers in React. According to the naming conventions, the function should be named onInputChange.
src/components/Dropdown/Dropdown.tsx
Outdated
| const handleInputFocus = () => { | ||
| setIsActive(true); | ||
| }; |
There was a problem hiding this comment.
The handleInputFocus function is not following the recommended naming conventions for event handlers in React. According to the naming conventions, the function should be named onInputFocus.
src/components/Dropdown/Dropdown.tsx
Outdated
| const handleSelect = (person: Person) => { | ||
| onSelect(person); | ||
| setQuery(person.name); | ||
| setIsActive(false); | ||
| }; |
There was a problem hiding this comment.
The handleSelect function is not following the recommended naming conventions for event handlers in React. According to the naming conventions, the function should be named onSelectPerson.
src/components/Dropdown/Dropdown.tsx
Outdated
| [appliedQuery], | ||
| ); | ||
|
|
||
| const visiblePeople = selectedPerson ? peopleFromServer : filteredPeople; |
There was a problem hiding this comment.
The key for list items is generated during render which is not a good practice. As per the article, it's better to generate unique keys for list items when the list is created.
src/components/Dropdown/Dropdown.tsx
Outdated
|
|
||
| const handleInputChange = (event: React.ChangeEvent<HTMLInputElement>) => { | ||
| setQuery(event.target.value.trimStart()); | ||
| applyQuery(event.target.value); |
There was a problem hiding this comment.
You need to make sure that filter won't be called if user entered spaces only. In the current implementation, if the user only enters spaces, the filter function will still be called. You should trim the input value before applying the filter.
| 'has-background-grey-lighter': selectedPerson === person, | ||
| })} | ||
| data-cy="suggestion-item" | ||
| onClick={() => onSelect(person)} |
There was a problem hiding this comment.
This is a performance issue. You are creating a new function every time the component renders. This is not recommended. Instead, create a function inside the component that accepts a person as an argument and call this function in your onClick handler. This way, a new function is not created on every render.
|
|
||
| return ( | ||
| <div | ||
| key={person.slug} |
There was a problem hiding this comment.
Key generation on render is not recommended. This is because keys should be stable between multiple renders for performance reasons. You can read more about it here.
| 'has-background-grey-lighter': selectedPerson === person, | ||
| })} | ||
| data-cy="suggestion-item" | ||
| onClick={() => onSelect(person)} |
There was a problem hiding this comment.
The naming convention for event handlers in React suggests that the name should start with 'handle' and end with the type of event you are handling. In this case, a better name would be 'handleSelectPerson'. You can read more about the naming conventions here.
src/utils/filterPeople.ts
Outdated
| ) { | ||
| let filteredPeople = [...people]; | ||
|
|
||
| if (appliedQuery) { |
There was a problem hiding this comment.
You should check if appliedQuery is not only truthy, but also not just spaces. Currently, if a user enters spaces only, the filter function will be called unnecessarily. Consider using trim() method before the check to ensure that spaces only won't pass the condition.
src/utils/filterPeople.ts
Outdated
| let filteredPeople = [...people]; | ||
|
|
||
| if (appliedQuery) { | ||
| const normalizedQuery = appliedQuery.toLocaleLowerCase().trim(); |
There was a problem hiding this comment.
The toLocaleLowerCase() method is a good approach to make the search case-insensitive. However, please note that for some languages and locales, this method might not work as expected. If you plan to support multiple languages, you might need to use a library for more accurate case conversion.
src/components/Dropdown/Dropdown.tsx
Outdated
|
|
||
| type Props = { | ||
| selectedPerson: Person | null; | ||
| onSelect: React.Dispatch<React.SetStateAction<Person | null>>; |
There was a problem hiding this comment.
I think - this will be better and more readable, right?
| onSelect: React.Dispatch<React.SetStateAction<Person | null>>; | |
| onSelect: (person: Person | null) => void; |
| const filteredPeople = useMemo( | ||
| () => filterPeople(peopleFromServer, { appliedQuery }), | ||
| [appliedQuery], | ||
| ); |
There was a problem hiding this comment.
change syntax and use it in the future(more readable)
| const filteredPeople = useMemo( | |
| () => filterPeople(peopleFromServer, { appliedQuery }), | |
| [appliedQuery], | |
| ); | |
| const filteredPeople = useMemo(() => ( | |
| filterPeople(peopleFromServer, { appliedQuery }) | |
| ), [appliedQuery]); |
There was a problem hiding this comment.
I'll remember it, thanks, but here prettier changes it back
src/components/Dropdown/Dropdown.tsx
Outdated
| [appliedQuery], | ||
| ); | ||
|
|
||
| const visiblePeople = selectedPerson ? peopleFromServer : filteredPeople; |
src/components/Dropdown/Dropdown.tsx
Outdated
| setIsActive(true); | ||
| }; | ||
|
|
||
| const onInputChange = (event: React.ChangeEvent<HTMLInputElement>) => { |
src/components/Dropdown/Dropdown.tsx
Outdated
| onSelect(null); | ||
| }; | ||
|
|
||
| const onSelectPerson = (person: Person) => { |
| @import '../../variables/colors'; | ||
|
|
||
| .dropdown-item:hover { | ||
| background-color: $dropdown-backgrount-hover-color; |
There was a problem hiding this comment.
syntax mistake
| background-color: $dropdown-backgrount-hover-color; | |
| background-color: $dropdown-background-hover-color; |
src/components/Dropdown/Dropdown.tsx
Outdated
| <DropdownMenu | ||
| people={visiblePeople} | ||
| selectedPerson={selectedPerson} | ||
| handleSelectPerson={onSelectPerson} |
There was a problem hiding this comment.
should be the other way round)
| handleSelectPerson={onSelectPerson} | |
| onSelectPerson={handleSelectPerson} |
There was a problem hiding this comment.
this was a correction from AI Buddy)
src/utils/filterPeople.ts
Outdated
| let filteredPeople = [...people]; | ||
| const normalizedQuery = appliedQuery.toLocaleLowerCase().trim(); | ||
|
|
||
| if (normalizedQuery.length) { | ||
| filteredPeople = filteredPeople.filter(person => | ||
| person.name.toLocaleLowerCase().includes(normalizedQuery), | ||
| ); | ||
| } | ||
|
|
||
| return filteredPeople; |
There was a problem hiding this comment.
method filter returns a new array. u don't need the let variable here. Avoid creating let variables
| let filteredPeople = [...people]; | |
| const normalizedQuery = appliedQuery.toLocaleLowerCase().trim(); | |
| if (normalizedQuery.length) { | |
| filteredPeople = filteredPeople.filter(person => | |
| person.name.toLocaleLowerCase().includes(normalizedQuery), | |
| ); | |
| } | |
| return filteredPeople; | |
| const normalizedQuery = appliedQuery.toLocaleLowerCase().trim(); | |
| return normalizedQuery.length | |
| ? people.filter((person) => ( | |
| person.name.toLocaleLowerCase().includes(normalizedQuery) | |
| )) | |
| : [...people]; |
There was a problem hiding this comment.
changed, but prettier removes brackets around the filter callback value
DEMO LINK