add task solution #1129
add task solution #1129
Conversation
There was a problem hiding this comment.
Great job on implementing the task requirements! 🎉 While there are some areas for improvement, such as ensuring correct URL formation and refining error handling, these are minor and do not impact the core functionality. Keep up the good work and consider the feedback for future enhancements! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| className="navbar-item has-background-grey-lighter" | ||
| href="#/people" | ||
| className={getNavLinkClasses} | ||
| to={{ pathname: 'people', search }} |
There was a problem hiding this comment.
The to prop in the NavLink component is being passed an object with pathname and search. While this is valid, ensure that the search variable is correctly formatted as a query string. If search is not a string, it may cause issues with URL formation.
| const [searchParams, setSearchParams] = useSearchParams(); | ||
|
|
||
| const currentSex = searchParams.get('sex') ?? ''; | ||
| const currentCenturies = searchParams.getAll('centuries') ?? []; |
There was a problem hiding this comment.
The use of searchParams.getAll('centuries') will return an array, which is correct. However, the ?? [] is unnecessary here because getAll will always return an array, even if it's empty.
|
|
||
| const handleReset = () => { | ||
| setQuery(''); | ||
| setSearchParams({}); |
There was a problem hiding this comment.
Calling setSearchParams({}) will reset all search parameters. Ensure this is the intended behavior, as it will clear all filters, not just the query.
| className={classNames('button', 'mr-1', { | ||
| 'is-info': currentCenturies.includes(century), | ||
| })} | ||
| params={{ centuries: handleToggleCentury(century) }} |
There was a problem hiding this comment.
The params prop in SearchLink for centuries might not work as expected if handleToggleCentury returns an empty array. Ensure that the logic for handling empty arrays is correctly managed in the SearchLink component.
|
|
||
| getPeople() | ||
| .then(response => setPeople(response)) | ||
| .catch(() => setHasError(true)) |
There was a problem hiding this comment.
Consider providing more detailed error handling in the catch block. Currently, it only sets hasError to true without any specific error message or logging. This could make debugging more difficult.
|
|
||
| return ( | ||
| <td> | ||
| <Link to={{ pathname: `/people/${father.slug}`, search }}> |
There was a problem hiding this comment.
Similar to the mother link, ensure that the search variable is correctly formatted as a query string to avoid potential URL issues.
| <Link | ||
| to={{ pathname: `/people/${slug}`, search }} | ||
| className={classNames({ | ||
| 'has-text-danger': sex === 'f', |
There was a problem hiding this comment.
The className has-text-danger is applied based on the sex being 'f'. Ensure this is the intended styling logic, as it might affect the visual representation of the link.
|
|
||
| <Route path="*" element={<h1 className="title">Page not found</h1>} /> | ||
|
|
||
| <Route path="home" element={<Navigate to=".." replace />} /> |
There was a problem hiding this comment.
The Navigate component is used to redirect from /home to .. (the parent route). Ensure that this redirection logic is correct and aligns with the intended navigation flow of the application.
| const YEARS_IN_CENTURY = 100; | ||
|
|
||
| people = people.filter(({ born }) => | ||
| centuries.includes(String(Math.ceil(born / YEARS_IN_CENTURY))), |
There was a problem hiding this comment.
The filtering logic for centuries assumes that the born year is always positive. Consider handling cases where born might be negative (e.g., BCE dates) if applicable to your dataset.
| if (order) { | ||
| people.reverse(); | ||
| } |
There was a problem hiding this comment.
The order parameter is used to reverse the list if present. Ensure that this logic aligns with the intended sorting behavior, as reversing might not be the correct action for all sorting scenarios.
There was a problem hiding this comment.
Great job on implementing the task requirements! 🎉 The code is well-structured and meets the core functionalities, including filtering and sorting with URL search parameters. While there are minor improvements suggested for sorting logic, they don't hinder the overall functionality. Keep up the good work and consider refining the sort logic for even better performance in future iterations! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| if (currentSort === sortBy && currentOrder === 'desc') { | ||
| return { sort: sortBy, order: null }; |
There was a problem hiding this comment.
The handleToggleSort function seems to have a logic error. When the current sort is the same as sortBy and the order is 'desc', it should toggle to 'asc' instead of setting the order to null. Consider changing the return statement to { sort: sortBy, order: 'asc' }.
| if (order) { | ||
| people.reverse(); |
There was a problem hiding this comment.
The logic for reversing the order is correct, but it assumes that the default sort order is ascending. Ensure that the order parameter is explicitly checked for 'desc' before reversing, to avoid unintended behavior if other order values are introduced.
DEMO LINK