Pronina-People-table-advanced#1831
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because several core task requirements around filtering, sorting, and URL search params are not implemented yet, which means the solution does not fulfill the specified behavior. The People page currently loads and displays data correctly, but it does not read or apply query, sex, centuries, sort, or order from useSearchParams, so the people array is never filtered or sorted as required [CHECKLIST ITEM #1–#3, #7–#14]. Navigation within /people (Navbar People link and person links in the table) uses plain Link/NavLink without preserving search params, so any active filters or sorting are lost when you navigate, violating the requirement to keep state in the URL [CHECKLIST ITEM #4, #16]. Also, PeopleFilters is rendered unconditionally instead of only when people are successfully loaded and non-empty, which breaks the sidebar visibility rule [CHECKLIST ITEM #5].
The critical fixes you need are:
- In
PeoplePage.tsx, useuseSearchParamsto derivequery,sex,centuries,sort, andorder, compute a filtered & sortedvisiblePeoplearray (case-insensitive name/mother/father filtering, multi-select centuries viagetAll, sort byname/sex/born/diedwith the 3-click cycle), and passvisiblePeopletoPeopleTableinstead of rawpeople. - Gate rendering of
PeopleFiltersbehind a condition like!loading && !error && people.length > 0so the sidebar only appears after data is loaded. - Update navigation within the People section (
NavbarPeople link and table/person links) to preserve the current search string, using a helper likeSearchLinkorto={{ pathname: '/people', search: location.search }}so filters and sort settings stay in the URL. - Refactor
PeopleFilters(in its own file) to stop using statichrefhash links and instead useSearchLink/getSearchWithto updatequery,sex, andcenturiessearch params.
Once these behaviors are wired up, your existing routing and table structure will nicely support the full feature set, and the app will align with all the checklist items. You already have a solid base (data loading, error handling, and routing), so focus on integrating the URL-driven filtering/sorting layer on top of it.
✨ 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
| <h1 className="title">Home Page</h1> | ||
| <h1 className="title">Page not found</h1> | ||
| <PeoplePage /> | ||
| <Routes> |
There was a problem hiding this comment.
Current routing works, but nothing here ensures that search params are preserved when navigating within /people (checklist #4/#16). When you later update links (e.g. People link in Navbar and person links in the table) to keep query/sort/centuries, make sure the router structure still supports that behavior.
| <Route | ||
| path="*" | ||
| element={<h1 className="title">Page not found</h1>} | ||
| /> |
There was a problem hiding this comment.
The People nav link doesn’t preserve current search params, so navigating back to /people from a person page will reset filters and sorting, violating checklist items #4 and #16. Consider using your SearchLink or useSearchParams to keep the existing search in the URL for this link when already on the People section.
| <NavLink | ||
| to="/people" |
There was a problem hiding this comment.
The requirements (#4, #16) specify that search params must be kept when navigating within the People page, but this NavLink always goes to /people without preserving any existing search string. Consider using a helper (like SearchLink with useSearchParams) or building the to with the current location.search so filters/sorting are not lost when clicking People in the navbar.
| </NavLink> | ||
| </div> |
There was a problem hiding this comment.
PeopleFilters is rendered unconditionally, but checklist item #5 requires the sidebar with filters to appear only when people are loaded. You should gate this component behind !loading && !error && people.length > 0 (or equivalent) so it doesn’t render before data is available.
| setLoading(true); | ||
| getPeople() | ||
| .then(data => { | ||
| const peopleWithParents = data.map(person => ({ |
There was a problem hiding this comment.
The current routing setup correctly renders PeoplePage for both /people and /people/:slug, but it doesn’t address checklist items #4 and #16: search params must be kept when navigating within the People page. You’ll need to ensure links to /people and person details use search-aware navigation (via SearchLink) so query/sort/centuries params are preserved.
| </th> | ||
|
|
||
| <th>Name</th> | ||
| <th>Sex</th> |
There was a problem hiding this comment.
The /people/:slug route reuses PeoplePage, but you are not preserving search params when navigating to a specific person, nor passing any slug-based context into filtered/sorted state. Checklist items #3, #4, #16 require that filters and sort params are saved in URL search params and kept when navigating within the People page.
| </tr> | ||
| </thead> | ||
|
|
||
| <tbody> |
There was a problem hiding this comment.
| ) : ( | ||
| person.motherName || '-' |
There was a problem hiding this comment.
PeoplePage currently only loads and stores people; it does not read or apply any URL search params (query, sex, centuries, sort, order) to filter/sort the people before passing them to PeopleTable, which is required by several checklist items.
| <th>Name</th> | ||
| <th>Sex</th> | ||
| <th>Born</th> | ||
| <th>Died</th> |
| <Link | ||
| to={`/people/${person.slug}`} |
There was a problem hiding this comment.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved because it fully meets the functional requirements for the advanced People Table task. Filtering, sorting, and navigation are all driven by URL search parameters (query, sex, centuries, sort, order), and the state is correctly restored on reload and when sharing URLs. The 3-step sorting cycle (ascending → descending → off) is implemented correctly, and when sorting is disabled, sort and order are removed from the URL as required. Navigation within the People section (including switching between list and details, and following person/mother/father links) preserves the current filters and sorting, which satisfies the “keep search params within /people” requirement. The sidebar only appears once people are successfully loaded and non-empty, and the filters behave correctly, including multi-century selection and removing query when the input is cleared. As a small optional improvement, you could consider using SearchLink consistently instead of manually concatenating ?${searchParams.toString()} in a few places to centralize URL-handling logic. Overall, this is a solid, well-structured implementation that aligns with the checklist and shows good understanding of URL-based state management.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
DEMO LINK