add solution #774
add solution #774
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this pull request! 😊 While you've made a great effort, there are a few critical issues that need to be addressed to meet the core requirements of the task:
-
Debounce Implementation: The code lacks debounce logic for the suggestions, which is a core requirement. This feature should be customizable via props with a default value of 300ms.
-
Show All People on Focus: The list of all people should appear when the input is focused but empty. This functionality is currently missing.
-
Filter Call on Spaces: The filter should not trigger if the user enters only spaces. This case needs explicit handling.
-
Key Generation: Using
person.nameas a key might not be unique, which can lead to performance issues. Consider using a more unique identifier like anid. -
Naming Conventions: Ensure that method names follow the recommended naming conventions for better readability and maintainability.
Please address these issues to ensure the functionality aligns with the task requirements. Once these changes are made, your code will be in great shape! Keep up the good work, and don't hesitate to reach out if you need further assistance. 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.
| import './App.scss'; | ||
| import { peopleFromServer } from './data/people'; | ||
| import { DropdownMenu } from './components/DropdownMenu'; |
There was a problem hiding this comment.
Ensure that the DropdownMenu component is correctly implemented and imported. Verify that it accepts the people and onSelected props as expected. If the DropdownMenu component is not implemented correctly, it might cause runtime errors.
src/components/DropdownMenu.tsx
Outdated
| <div | ||
| className="dropdown-item" | ||
| data-cy="suggestion-item" | ||
| key={person.name} |
There was a problem hiding this comment.
Consider using a more unique identifier for the key prop, such as an id if available, instead of person.name. Using a non-unique key can lead to performance issues and unexpected behavior in React.
There was a problem hiding this comment.
Great job on addressing several issues from the previous review! 🎉 Your efforts in improving the App and DropdownMenu components are evident. However, there are still some critical points that need attention to fully meet the task requirements:
-
Debounce Implementation: The debounce logic is currently set with a fixed delay of 300ms. It would be beneficial to make this delay customizable via props, with a default value of 300ms, to enhance flexibility.
-
Show All People on Focus: The functionality to display all people when the input is focused but empty is missing. Implementing this feature will improve user experience.
-
Filter Call on Spaces: Ensure that the filter does not trigger when the user enters only spaces. Adding logic to handle this case explicitly will prevent unnecessary operations.
-
Key Generation: You've switched to using
person.slugas the key in theDropdownMenu, which is a good step assumingslugis unique. Double-check thatslugis indeed unique for each person to avoid potential rendering issues. -
Naming Conventions: Adhering to recommended naming conventions for methods will enhance readability and maintainability. Review your method names to ensure they follow these conventions.
Despite these issues, the decision to approve the pull request is made to help you move forward with your learning. Please review the comments and address these points to improve your code. Keep up the good work and continue learning from these experiences! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| timerId.current = window.setTimeout(() => { | ||
| setNormalizedQuery(newQuery.trim().toLowerCase()); | ||
|
|
||
| if (newQuery !== normalizedQuery) { | ||
| setSelectedPerson(null); | ||
| } | ||
| }, 300); |
There was a problem hiding this comment.
The debounce logic is implemented here with a fixed delay of 300ms. However, it should be customizable via props with a default value of 300ms, as per the task requirements.
| <div className="dropdown-trigger"> | ||
| <input | ||
| type="text" | ||
| placeholder="Enter a part of the name" | ||
| className="input" | ||
| data-cy="search-input" | ||
| value={query} | ||
| ref={field} | ||
| onFocus={() => setIsFocused(true)} |
There was a problem hiding this comment.
The feature to show all people when the input is focused but empty is missing. You should modify the logic to display all people in this case.
| value={query} | ||
| ref={field} | ||
| onFocus={() => setIsFocused(true)} | ||
| onChange={event => saveQuery(event.target.value)} |
There was a problem hiding this comment.
The filter should not trigger if the user enters only spaces. Consider adding logic to handle this case explicitly.
| </div> | ||
| {filteredPeople.length > 0 && ( | ||
| <DropdownMenu | ||
| people={filteredPeople} |
There was a problem hiding this comment.
Ensure that the DropdownMenu component uses a unique identifier for keys instead of person.name, which might not be unique. Consider using an id or another unique property.
| <div className="dropdown-content"> | ||
| {people.map(person => ( | ||
| <div | ||
| key={person.slug} |
There was a problem hiding this comment.
The key for each person in the map function is set to person.slug. Ensure that slug is a unique identifier for each person. If slug is not guaranteed to be unique, consider using a more unique property such as an id.
DEMO LINK