Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on the overall structure and core functionality! However, a few changes are needed to meet all the requirements. The most critical issue is that the onSelected callback prop is not being used correctly in Autocomplete.tsx, which prevents the selected person's name from appearing in the h1 tag. Additionally, your solution violates a couple of checklist items: the filter runs on input with only spaces [CHECKLIST ITEM #1], and an event handler has an unconventional name [CHECKLIST ITEM #4]. Lastly, for better accessibility in Item.tsx, the onClick handler should be on the clickable div element rather than the <p> tag. Please address these points, and your component will be ready!
✨ 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
| }, delay); | ||
| }, [delay]); | ||
|
|
||
| const showDropManu = (): void => { |
There was a problem hiding this comment.
This violates checklist item #4: 'follow [these] naming conventions for methods'. Since this function is an event handler for the onFocus event, a more conventional name would be handleFocus.
| return peopleFromServer.filter(el => | ||
| el.name.toLowerCase().includes(valueInp.toLowerCase()), | ||
| ); |
There was a problem hiding this comment.
This violates checklist item #1: 'make sure that filter won't be called if user entered spaces only'. Currently, the filter runs even if the input string consists only of whitespace. You should add a check to handle this case, for example, by trimming the input value and not running the filter if the result is an empty string.
| onClick={() => { | ||
| onSelect(user); | ||
| }} |
There was a problem hiding this comment.
For better semantics and accessibility, the onClick handler should be placed on the parent div which has the is-clickable class. A <p> tag is not an interactive element by default, which means it's not keyboard-accessible (e.g., users can't tab to it). The div with the dropdown-item class represents the entire clickable suggestion.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Changes are needed to fully meet the requirements. The filter logic still runs on whitespace-only input (e.g., ' '), violating [CHECKLIST ITEM #1] — the component should treat such input as empty and skip filtering entirely. While getFilteredPeople correctly trims and checks !normalizedValue, the rendering condition query !== '' in List.tsx causes incorrect behavior: it shows the list or 'No suggestions' even when only spaces are entered. You must update the rendering logic to use the same normalized check (!normalizedValue) consistently across both filtering and display decisions. Also, please include src/components/Item.tsx in your next submission so I can verify the accessibility fix for the onClick handler placement [CHECKLIST ITEM #2]. Everything else — debounce, selection handling, callback propagation, and empty-state behavior — is implemented correctly and well-structured.
✨ 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
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this iteration! I am approving your solution to help you progress, but there is one important accessibility improvement to address.
In src/components/Item.tsx, using a div with an onClick handler makes the component inaccessible to keyboard users. Interactive elements should use semantic tags like <button> or <a>. For Bulma's dropdown-item, an <a> tag is the recommended and accessible choice [CHECKLIST ITEM #2].
You did a fantastic job resolving the previous issue with whitespace handling, and the component now works perfectly. I also left a small suggestion in src/components/Autocomplete.tsx to make the rendering logic a bit cleaner. Keep up the great momentum!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| {!normalizedQuery && ( | ||
| <List result={peopleFromServer} onSelect={handleSelect} /> | ||
| )} | ||
|
|
||
| {normalizedQuery && result.length > 0 && ( | ||
| <List result={result} onSelect={handleSelect} /> | ||
| )} | ||
|
|
||
| {normalizedQuery && result.length === 0 && <NoSuggestions />} |
There was a problem hiding this comment.
This rendering logic is correct, but it can be simplified. The result variable already contains the correct list to display, whether it's the full list for an empty query or a filtered list. You can combine the first two conditions into a single check.
Consider this simplified version:
{result.length > 0 && (
<List result={result} onSelect={handleSelect} />
)}
{normalizedQuery && result.length === 0 && <NoSuggestions />}This refactoring removes the redundant condition and makes the code's intent clearer, as result is already correctly calculated to be either the full list or the filtered list based on the query.
2 participants
No description provided.