done #768
done #768
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this pull request! 😊 While your code is mostly well-structured and meets many of the task requirements, there are a couple of critical issues that need to be addressed before we can approve it:
-
Filter on Spaces: The current implementation calls the
filterSuggestionsfunction even when the input is just spaces. This does not align with the task requirements, which specify that the filter should not be triggered by spaces alone. Please update the logic to handle this case. -
Key Generation: Ensure that
person.slugis a unique and stable identifier for each suggestion. Ifslugis not guaranteed to be unique, it could lead to performance issues and bugs in React. Consider using a different unique identifier if necessary. -
JSX Syntax Error: There is a syntax error in your JSX expression. The ternary operator should be wrapped in curly braces to be evaluated correctly. Please fix this to ensure the component renders as expected.
Once these issues are resolved, your code will be in great shape! Keep up the great work, and don't hesitate to reach out if you have any questions or 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.
| {suggestions.length > 0 ? ( | ||
| suggestions.map(person => ( | ||
| <div | ||
| key={person.slug} |
There was a problem hiding this comment.
Ensure that person.slug is a unique identifier for each Person. Using a non-unique key can lead to performance issues and bugs in React. If slug is not guaranteed to be unique, consider using a different unique identifier.
There was a problem hiding this comment.
Great job on implementing the main functionality of your components! 🎉 Your code meets most of the task requirements, and it's clear you've put a lot of effort into it. Here are a few points to consider for improvement:
-
Filter on Spaces: Ensure that the filter function is not called when the input consists only of spaces. This will enhance the performance and user experience.
-
Key Generation: Verify that
person.slugis a stable and unique identifier to prevent potential issues with key generation during rendering. -
Naming Conventions: Review your method names to align with recommended naming conventions for better code clarity.
-
Conditional Rendering: Use optional chaining or explicit checks when accessing properties of
selectedPersonto avoid runtime errors. -
Function Responsibilities: Consider refactoring the
handleInputChangefunction to keep it focused on its primary responsibility, and move unrelated logic elsewhere.
These improvements will not only help you comply with the task requirements but also enhance the overall quality of your code. Keep up the great work, and use this feedback to continue growing your skills! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| {selectedPerson | ||
| ? `${selectedPerson.name} (${selectedPerson.born} - ${selectedPerson.died})` | ||
| : 'No selected person'} |
There was a problem hiding this comment.
The conditional rendering of selectedPerson is correct, but ensure that selectedPerson is not null before accessing its properties. The current implementation assumes selectedPerson is not null when accessing name, born, and died. Consider using optional chaining or a more explicit check to avoid potential runtime errors.
| if (selectedPerson) { | ||
| setSelectedPerson(null); | ||
| onSelected(null); | ||
| } |
There was a problem hiding this comment.
The check for selectedPerson before setting it to null and calling onSelected(null) is a good practice to avoid unnecessary state updates. However, consider moving this logic outside of the handleInputChange function if it's not directly related to input changes, to keep the function focused on its primary responsibility.
| if (!inputValue) { | ||
| setSuggestions(people); | ||
| setIsDropDownActive(true); | ||
| } |
There was a problem hiding this comment.
The handleInputFocus function sets suggestions to all people when the input is focused and empty. This is a good approach to show all options initially, but ensure that this behavior aligns with the desired user experience. If not, consider adjusting the logic to better fit the requirements.
| {selectedPerson | ||
| ? `${selectedPerson.name} (${selectedPerson.born} - ${selectedPerson.died})` | ||
| : 'No selected person'} |
There was a problem hiding this comment.
Move this logic to the helper variable and use it here
| }; | ||
|
|
||
| return ( | ||
| <div className={`dropdown ${isDropDownActive ? 'is-active' : ''}`}> |
There was a problem hiding this comment.
Use the classnames library for add classes with condition
DEMO LINK
btw this was too hard for me 😿