solution #1133
solution #1133
Conversation
There was a problem hiding this comment.
Great job on implementing the task requirements! 🎉 Your code meets the core functionality and constraints, and the overall structure is well-organized. While there are a few minor issues related to parameter handling and assumptions about data uniqueness, these don't significantly impact the core functionality. Consider addressing these in future iterations to enhance robustness and maintainability. Keep up the great work! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <Route path="people" element={<PeoplePage />}> | ||
| <Route path=":slug" element={<PeoplePage />} /> | ||
| </Route> |
There was a problem hiding this comment.
The nested route for PeoplePage is defined, but it might be worth checking if the PeoplePage component is designed to handle both the base path /people and the nested :slug path. If PeoplePage is not intended to handle both, consider creating a separate component for the nested route.
| const resetParams = { | ||
| query: null, | ||
| sex: null, | ||
| centuries: null, | ||
| }; |
There was a problem hiding this comment.
The resetParams object sets query, sex, and centuries to null. However, when using URLSearchParams, setting a parameter to null does not remove it from the URL. Consider using params.delete('key') to remove parameters or set them to an empty string if you want to clear them.
| ? { sort: null, order: null } | ||
| : { sort: value, order: 'desc' }; |
There was a problem hiding this comment.
In the sortParams function, setting sort and order to null does not remove them from the URL. Consider using params.delete('key') to remove these parameters or set them to an empty string if you want to clear them. This will ensure the URL reflects the absence of sorting when the same field is clicked twice.
| const mother = people.find(mom => mom.name === person.motherName); | ||
| const father = people.find(dad => dad.name === person.fatherName); |
There was a problem hiding this comment.
The logic for finding a person's mother and father assumes that names are unique identifiers. If there are multiple people with the same name, this could lead to incorrect parent assignments. Consider using a unique identifier, such as an ID, to find parents accurately.
| sort: string | null; | ||
| order: string | null; |
There was a problem hiding this comment.
The sort and order parameters are typed as string | null, but they are defaulted to an empty string in the calling code. Ensure consistency by either allowing empty strings here or adjusting the calling code to use null.
| if (sort) { | ||
| filteredPeople.sort((currentPerson, nextPerson) => { | ||
| switch (sort) { | ||
| case 'name': | ||
| return order === 'desc' | ||
| ? nextPerson.name.localeCompare(currentPerson.name) | ||
| : currentPerson.name.localeCompare(nextPerson.name); | ||
| case 'sex': | ||
| return order === 'desc' | ||
| ? nextPerson.sex.localeCompare(currentPerson.sex) | ||
| : currentPerson.sex.localeCompare(nextPerson.sex); | ||
| case 'born': | ||
| return order === 'desc' | ||
| ? nextPerson.born - currentPerson.born | ||
| : currentPerson.born - nextPerson.born; | ||
| case 'died': | ||
| return order === 'desc' | ||
| ? nextPerson.died - currentPerson.died | ||
| : currentPerson.died - nextPerson.died; | ||
| default: | ||
| return 0; | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
The sorting logic assumes that all fields are present and comparable. Consider adding checks to ensure that fields like born and died are not undefined before performing arithmetic operations, to avoid potential runtime errors.
DEMO LINK