add solution#1826
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 one critical requirement about preserving URL search params within the People page is still not met. Specifically, the People navigation link in src/components/Navigation/Navigation.tsx uses a plain Link with to="/people", which clears existing filters and sort parameters; this violates [CHECKLIST ITEM #4] and [CHECKLIST ITEM #15], which require keeping search params when navigating within the People page. Similarly, in src/components/PeopleFilters/PeopleFilters.tsx, the Link that goes to /people also drops the current search params, causing the same issue.
To fix this, you should update both of these links so they preserve the current search string when navigating to /people. For example, you can either: 1) replace these Link components with your SearchLink and pass the current params, or 2) use useSearchParams (or useLocation) and set to={{ pathname: '/people', search: searchParams.toString() }} so all filters and sort options remain in the URL. Apart from this, your filtering, sorting, and search-param handling are implemented very well and align with the rest of the checklist. Once you ensure the People links keep the existing search params, your solution will fully satisfy the task requirements.
✨ 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
| <Link | ||
| to="/people" |
There was a problem hiding this comment.
The People nav link uses a plain to="/people", which drops existing search params; checklist items #4 and #15 require keeping filters/sort params when navigating within the People page, so this link should preserve the current search string (e.g., by using SearchLink or including search from useLocation/useSearchParams).
|
|
||
| function toggleCentury(century: string) { |
There was a problem hiding this comment.
This Link to /people always drops existing search params (filters/sort), which conflicts with checklist items #4 and #15 that require keeping search params when navigating within the People page. Consider using SearchLink or useSearchParams with getSearchWith so this link preserves the current query string.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution because all critical functional requirements for the People table are correctly implemented and wired through URL search params. Filtering by name, sex, and multiple centuries is fully URL-driven, case-insensitive where required, and correctly removes params when filters are cleared, so reloading or sharing the link reproduces the current view. Sorting by name, sex, born, and died follows the required three-step cycle (ascending → descending → off) and updates/removes the sort/order params exactly as specified, with SortIcon accurately reflecting the current state. Navigation within the People section preserves search params both when selecting a person (PersonLink with searchParams.toString()) and when using the People link in the navbar, so filters and sorting are not lost while moving between /people and /people/:slug. The filters sidebar is correctly shown only after people are successfully loaded and hidden for loading, error, or no-data states, and the empty-results messaging for “no data on server” vs “no matches for current filters” is handled cleanly. As minor, non-blocking polish, you could consider having “Reset all filters” also clear sorting, and update the HTML page title to better match the app, but these are enhancements rather than required fixes. Overall, this is a solid, requirement-compliant implementation with clear, consistent use of React Router search params and good UX behavior.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| <div className="column is-7-tablet is-narrow-desktop"> | ||
| {!loading && !error && people.length > 0 && <PeopleFilters />} |
There was a problem hiding this comment.
This condition correctly ensures the filters sidebar is only shown after people have been loaded successfully and there is at least one person, which matches the requirement that the sidebar should appear only when people are loaded.
| const search = searchParams.get('query') || ''; | ||
| const sex = searchParams.get('sex') || ''; | ||
| const centuries = searchParams.getAll('centuries'); | ||
| const sort = searchParams.get('sort') || ''; | ||
| const order = searchParams.get('order') || ''; |
There was a problem hiding this comment.
Here you read query from the URL and later filter name, motherName, and fatherName case-insensitively. Together with NameFilter writing to query, this satisfies the requirement for name-based filtering via URL search params.
| if (sort) { | ||
| result.sort((a, b) => { | ||
| switch (sort) { | ||
| case 'name': | ||
| case 'sex': | ||
| return a[sort].localeCompare(b[sort]); | ||
| case 'born': | ||
| case 'died': | ||
| return a[sort] - b[sort]; | ||
| default: | ||
| return 0; | ||
| } | ||
| }); | ||
|
|
||
| if (order === 'desc') { | ||
| result.reverse(); | ||
| } | ||
| } |
There was a problem hiding this comment.
This block implements sorting by name, sex, born, and died, and uses order === 'desc' to reverse the result. It aligns with the 3-step sorting behavior when combined with getNextSortParams and the URL sort/order params.
| function getNextSortParams(field: string) { | ||
| if (sort !== field) { | ||
| return { sort: field, order: null }; | ||
| } | ||
|
|
||
| if (sort === field && order !== 'desc') { | ||
| return { sort: field, order: 'desc' }; | ||
| } | ||
|
|
||
| return { sort: null, order: null }; | ||
| } |
There was a problem hiding this comment.
getNextSortParams correctly implements the three-step toggle: first click sets ascending for this field, second click sets descending, and third click clears both sort and order (so they are removed from the URL via SearchLink/getSearchWith).
| {sortFields.map(field => ( | ||
| <th key={field}> | ||
| <span className="is-flex is-flex-wrap-nowrap"> | ||
| {field[0].toUpperCase() + field.slice(1)} | ||
| <SearchLink params={getNextSortParams(field)}> | ||
| <SortIcon | ||
| sortField={field} | ||
| currentSort={sort} | ||
| currentOrder={order} | ||
| /> | ||
| </SearchLink> |
There was a problem hiding this comment.
Using SearchLink here with getNextSortParams(field) wires the header icons to the URL-based sorting logic, so clicks update sort/order in search params while preserving other filters.
3 participants
https://MaxShaggy.github.io/react_people-table-advanced/