Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This submission is well-structured, but it requires changes because a couple of core functional requirements are not met. The most critical issue is that the dropdown list does not close after a user selects an item. In UserSelect.tsx, the onClick handler for a suggestion must also update the state to close the dropdown. Additionally, the task requires the callback prop to be named onSelected, but it has been implemented as onSelect. Please address these two points to fully meet the requirements.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| <input | ||
| type="text" | ||
| placeholder="Enter a part of the name" | ||
| className="input" | ||
| data-cy="search-input" | ||
| value={query} | ||
| onChange={handleQueryChange} | ||
| onFocus={() => setIsDropdownOpen(true)} | ||
| /> |
There was a problem hiding this comment.
For a more intuitive user experience, consider adding an onBlur event handler to this input. This would allow you to close the dropdown (setIsDropdownOpen(false)) when the user clicks away from the component.
| onClick={() => { | ||
| onSelect(person); | ||
| setQuery(person.name); | ||
| }} |
There was a problem hiding this comment.
The requirement to 'close the list' after selecting an item is not met. You should update the state to close the dropdown here.
Additionally, appliedQuery is not updated after selection. This can lead to showing a stale list of suggestions if the user reopens the dropdown. It would be better to also update appliedQuery to match the selected person's name.
src/components/UserSelect.tsx
Outdated
| )} | ||
| </div> | ||
|
|
||
| {filterPeople.length === 0 && ( |
There was a problem hiding this comment.
The condition for showing this message could be more specific. Currently, it will be displayed on initial render if the people prop is an empty array. It's better to show this message only when a user has actually searched for something. A condition like appliedQuery.trim().length > 0 && filterPeople.length === 0 would be more robust.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Good progress on the component's structure and filtering logic. However, this requires changes because a few critical requirements are still pending from the previous review.
- The callback prop must be named
onSelectedas specified in the task description. Please update this inUserSelect.tsxand its usage inApp.tsx. - The dropdown list does not close after an item is selected. This is a core requirement and can be fixed by updating the state to hide the dropdown within the selection handler.
- Currently, clicking a suggestion doesn't work because the input's
onBlurevent closes the dropdown before the item'sonClickcan register. Consider usingonMouseDownon the suggestion items to fix this, as it fires beforeonBlur.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
src/App.tsx
Outdated
| > | ||
| <p className="has-text-danger">No matching suggestions</p> | ||
| </div> | ||
| <UserSelect people={peopleFromServer} onSelect={handleSelect} /> |
There was a problem hiding this comment.
As per the task requirements and previous feedback, the callback prop should be named onSelected. Please update it here and in the UserSelect component definition.
src/components/UserSelect.tsx
Outdated
| onSelect?: (person: Person | null) => void; | ||
| }; | ||
|
|
||
| export const UserSelect = ({ | ||
| people, | ||
| delay = 300, | ||
| onSelect = () => {}, |
There was a problem hiding this comment.
The task description requires the callback prop to be named onSelected. Please update the prop name here and where the component is used.
| onClick={() => { | ||
| onSelect(person); | ||
| setQuery(person.name); | ||
| setAppliedQuery(person.name); | ||
| }} |
There was a problem hiding this comment.
The dropdown list should close after a user selects an item, as required by the task. You can achieve this by updating the state that controls the dropdown's visibility inside this onClick handler.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I'm approving this submission to help you keep moving forward, but there's one important bug we need to fix. You've done an excellent job addressing the previous feedback, like renaming the prop to onSelected and closing the dropdown on selection!
The final issue is that clicking a suggestion doesn't work. This is because the input's onBlur event fires before the suggestion item's onClick event, closing the list prematurely.
To fix this, try replacing
onClickwithonMouseDownon your suggestion items insrc/components/UserSelect.tsx. Themousedownevent fires beforeblur, which will resolve the issue.
You're very close to a complete solution. Keep up the fantastic effort!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| value={query} | ||
| onChange={handleQueryChange} | ||
| onFocus={() => setIsDropdownOpen(true)} | ||
| onBlur={() => setIsDropdownOpen(false)} |
There was a problem hiding this comment.
The onBlur handler here causes a problem: when you click on a suggestion, this onBlur event fires before the suggestion's onClick event on line 61. This closes the dropdown, preventing the onClick from ever firing, which means suggestions can't be selected.
As mentioned in the previous review, replacing onClick with onMouseDown on the suggestion items will fix this because mousedown fires before blur.
2 participants
No description provided.