realize react_peoplt-table-advanced#1829
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 there is one functional issue that can break the required URL‑driven behavior for filters. In src/components/PeoplePage.tsx, you call setSearchParams(getSearchWith(...)), but setSearchParams expects either a URLSearchParams instance or an object of key–value pairs, while getSearchWith returns a plain string; this means the search params are not updated in the way React Router expects and can break NameFilter and other URL‑based behavior (related to checklist items #3, #6, #8, #17). To fix this, wrap the string from getSearchWith before passing it in, for example: setSearchParams(new URLSearchParams(getSearchWith(searchParams, { /* params */ }))), or change getSearchWith usage here so you pass a proper object/URLSearchParams instead of a raw string. Aside from this, your routing, navbar search preservation, filters (NameFilter, sex, centuries), and sorting behavior (PeopleTable + getPreparedPeople) align well with the specified requirements and are nicely implemented. Once you correct the setSearchParams usage so URL params are updated correctly, the solution will satisfy the core functional requirements. Keep the rest of your approach as is; it’s very close and fixing this one detail will make the behavior robust and consistent.
✨ 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
| className="navbar-item has-background-grey-lighter" | ||
| href="#/people" | ||
| > | ||
| <NavLink className={getLinkClass} to={{ pathname: '/people', search }}> |
There was a problem hiding this comment.
Using useLocation and passing search into the People NavLink is the correct way to preserve current search params when navigating to the People page, which fulfills the requirement to keep search params within the People page. Just make sure this pattern is also respected when adding any other internal links there.
| <NavLink className={getLinkClass} to={{ pathname: '/people', search }}> | ||
| People | ||
| </a> | ||
| </NavLink> |
There was a problem hiding this comment.
setSearchParams(getSearchWith(searchParams, { query: newQuery || null })) correctly updates the query param and removes it when the input is empty, matching the NameFilter URL behavior requirement. Ensure similar || null handling for any future optional params you might introduce.
| }, []); | ||
|
|
||
| const [searchParams] = useSearchParams(); | ||
|
|
There was a problem hiding this comment.
setSearchParams expects either an object mapping keys to string values or a URLSearchParams instance, but getSearchWith returns a plain string here. This means the URL search params are not being updated in the way React Router expects, which can break the NameFilter behavior (checklist items #3, #6, #8, #17). Consider creating a new URLSearchParams from getSearchWith(...) and passing that into setSearchParams.
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 it fully meets the functional requirements for the advanced People Table. All filters (sex, query, centuries) and sorting (sort, order) are correctly driven by URL search params, and you handle adding, updating, and removing these params exactly as specified. Navigation within the People section preserves the current search params from the navbar and when clicking into a person’s details, so state is not lost across route changes. The NameFilter and CenturyFilter behaviors are correctly implemented: the query is case-insensitive across name/mother/father and removed when empty, and multiple centuries can be toggled or cleared using getAll/arrays. Sorting for name, sex, born, and died implements the 3‑click cycle (asc → desc → off), and the URL accurately reflects and controls the sort state. I don’t see any critical blockers or missing checklist items, only potential minor UX or testing refinements that are optional. Well done bringing everything together cleanly and fixing the previous setSearchParams issue by passing a URLSearchParams instance.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.