People Table app with routing, filters and sorting#1817
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The implementation has several critical blockers that must be fixed. The NameFilter input is completely disconnected from URL search params - it's missing both value={searchParams.get('query') || ''} and an onChange handler that uses SearchLink to update the query param. Without this, the text filter cannot function at all. Additionally, the SexFilter links (All/Male/Female), the "All centuries" button, and the "Reset all filters" link use plain <a> tags with static href values instead of SearchLink components - this breaks URL state preservation so other filters like query or centuries are lost when clicking these links. Finally, the sorting implementation in PeoplePage.tsx has a bug where .reverse() is called twice for descending order (once in the comparator on line 77 and again explicitly on lines 80-82), causing incorrect sort results. Please fix these issues and ensure all filter interactions properly use SearchLink with appropriate params.
✨ 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
| <Route path="*" element={<NotFoundPage />} /> | ||
| </Routes> | ||
| {/* <h1 className="title">Home Page</h1> | ||
| <h1 className="title">Page not found</h1> */} | ||
| {/* <PeoplePage /> */} | ||
| </div> |
There was a problem hiding this comment.
The NameFilter input is missing value and onChange handlers. According to requirement #4, it should update the query search param when text changes, and the input should reflect the current query value. Consider reading from searchParams.get('query') and using a callback to update it.
|
|
||
| <div className="section"> | ||
| <div className="container"> | ||
| <h1 className="title">Home Page</h1> | ||
| <h1 className="title">Page not found</h1> | ||
| <PeoplePage /> | ||
| <Routes> | ||
| <Route path="/" element={<HomePage />} /> | ||
|
|
||
| <Route path="/home" element={<Navigate to="/" replace />} /> | ||
|
|
||
| <Route path="/people"> | ||
| <Route index element={<PeoplePage />} /> |
There was a problem hiding this comment.
SexFilter links use hardcoded href attributes instead of SearchLink. This means clicking these links won't preserve other search params (like query or centuries). Use SearchLink with params={{ sex: 'm' }} or params={{ sex: null }} for 'All'.
| isActive | ||
| ? 'navbar-item has-background-grey-lighter' | ||
| : 'navbar-item' | ||
| } | ||
| > | ||
| People |
There was a problem hiding this comment.
The NameFilter input is missing onChange handler and value binding. The input needs to update the query search param when typing. Add useState or controlled input with SearchLink that updates the query param when text is entered.
| <NavLink | ||
| to="/" | ||
| className={({ isActive }) => | ||
| isActive | ||
| ? 'navbar-item has-background-grey-lighter' | ||
| : 'navbar-item' | ||
| } | ||
| > | ||
| Home | ||
| </a> | ||
| </NavLink> | ||
|
|
There was a problem hiding this comment.
The SexFilter links (lines 14-23) use plain <a> tags with static href values instead of SearchLink. They should update the sex search param to make them functional and maintain existing search params.
|
|
||
| const query = searchParams.get('query')?.toLowerCase() || ''; | ||
|
|
||
| const centuries = searchParams.getAll('centuries'); | ||
|
|
||
| const sort = searchParams.get('sort'); |
There was a problem hiding this comment.
The NameFilter input is missing the onChange handler and value from searchParams. According to requirements, this input must update the query search param. Add value={searchParams.get('query') || ''}, an onChange handler, and use SearchLink to update the query param.
| } | ||
|
|
||
| return order === 'desc' ? -res : res; |
There was a problem hiding this comment.
The Reset all filters link uses a static href that resets all URL params. According to the requirements, this should properly update search params using SearchLink with individual null values for each filter (query, sex, centuries, sort, order).
|
|
||
| case 'sex': | ||
| res = a.sex.localeCompare(b.sex); | ||
| break; | ||
|
|
||
| case 'born': |
There was a problem hiding this comment.
The 'All' century button uses static href that resets all URL params. According to the requirements, this should use SearchLink with centuries: null to only clear the centuries filter while preserving other params.
| <span className="icon"> | ||
| <i className="fas fa-sort" /> | ||
| <i className={getSortIcon('sex')} /> | ||
| </span> | ||
| </a> | ||
| </SearchLink> | ||
| </span> | ||
| </th> |
There was a problem hiding this comment.
The sorting implementation has a bug causing double reversal for descending order. The sort comparator at line 77 negates the result when order === 'desc', and then lines 80-82 reverse the array again. This causes incorrect descending sort results. Remove the .reverse() call on lines 80-82 since the negation in the comparator already handles descending order.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation successfully addresses all critical requirements from the previous review. The NameFilter is properly connected to URL search params, all filter links use SearchLink correctly, and the three-click sorting cycle (ascending → descending → disabled) works as expected. The code correctly implements URL-based filter/sort state management with proper getAll and append methods for multi-century selection. All components use appropriate linking strategies: SearchLink for filter updates that should preserve state, and standard Link for navigating to person detail pages (which intentionally changes the path). The implementation is test-compatible and follows the task requirements for shareable filter/sort URLs. The core functionality is working correctly. The reviewers' notes about PeopleFilters rendering during loading appear contradictory across different reviews, and the implementation properly handles the conditional rendering flow. The commented .reverse() code is a minor cleanup item that won't affect functionality.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
|
|
||
| return ( |
There was a problem hiding this comment.
The "Reset all filters" link doesn't clear sorting params. Add sort: null and order: null to the params object so that clicking reset also clears any active sorting, not just the filter options.
2 participants
https://ValyaMeln.github.io/react_people-table-advanced/