Added filtering and sorting with searchParams#1833
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 are still a few requirement-level issues that affect core functionality. First, the Navbar People link and the "Reset all filters" link in PeopleFilters.tsx both navigate to /people without preserving existing search params, which violates the requirement that navigation within the People page must keep URL search params so filters and sort state are preserved [CHECKLIST ITEM #3]. You can fix this by deriving the current location.search or using SearchLink/setSearchParams to either keep the current search string or explicitly clear only the filter-related params while staying on the same pathname.
Second, the PeopleTable headers for Mother and Father are still wrapped in SearchLink, so clicking them sets sort=mother or sort=father, even though the task only allows sorting by name, sex, born, and died [CHECKLIST ITEM #9]. This can produce invalid sort keys for sortPeople, so those columns should not be sortable at all (skip SearchLink and sort-param changes for them). Third, PeoplePage renders PeopleFilters whenever isLoading is false, even if there is a loading error, a “no people” error, or an empty list; the requirement says the filters sidebar should appear only when people are successfully loaded [CHECKLIST ITEM #4], so you should gate PeopleFilters with conditions like !errors.loading && !errors.people && people.length > 0. Once you address these points, the rest of your filtering, sorting, and URL search-param logic looks solid, and your solution will align well with the advanced table 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
| <NavLink | ||
| aria-current="page" | ||
| className="navbar-item has-background-grey-lighter" | ||
| href="#/people" | ||
| className={({ isActive }) => | ||
| cn('navbar-item', { 'has-background-grey-lighter': isActive }) | ||
| } | ||
| to="/people" |
There was a problem hiding this comment.
Current People NavLink always navigates to /people and discards any existing search params, which violates checklist item #3 about keeping search params when navigating within the People page. Consider using useLocation or useSearchParams and generating a to that includes the existing search string so the filters/sort state are preserved when clicking this link.
|
|
||
| <div className="panel-block"> | ||
| <a className="button is-link is-outlined is-fullwidth" href="#/people"> | ||
| <Link className="button is-link is-outlined is-fullwidth" to="/people"> |
There was a problem hiding this comment.
Using a plain Link to /people for "Reset all filters" drops all existing search params, which conflicts with checklist item #3 (search params must be preserved when navigating within the People page). Consider resetting filters by updating search params (e.g. via SearchLink or setSearchParams with cleared values) while keeping the same pathname rather than hard-navigating to /people.
| {columnNames.map(columnName => { | ||
| const normalizedColumnName = columnName.toLowerCase(); | ||
|
|
||
| const searchParam = | ||
| search.sort === normalizedColumnName | ||
| ? search.order === 'desc' | ||
| ? { sort: null, order: null } | ||
| : { sort: normalizedColumnName, order: 'desc' } | ||
| : { sort: normalizedColumnName, order: null }; | ||
|
|
||
| return ( | ||
| <th key={columnName}> | ||
| <span className="is-flex is-flex-wrap-nowrap"> | ||
| {columnName} | ||
| <SearchLink params={searchParam}> | ||
| { | ||
| <SortIcon | ||
| columnName={columnName} | ||
| currentSort={search.sort} | ||
| currentOrder={search.order} | ||
| /> | ||
| } | ||
| </SearchLink> | ||
| </span> | ||
| </th> |
There was a problem hiding this comment.
Although SortIcon returns null for Mother/Father, this header still wraps SearchLink using normalizedColumnName, so clicking those headers will set sort to mother or father. The requirements only specify sorting by name, sex, born, and died (checklist item #9). Consider skipping SearchLink and any sort-param changes for Mother and Father columns so they are not sortable.
| <div className="column is-7-tablet is-narrow-desktop"> | ||
| {isLoading ? <Loader /> : <PeopleFilters />} |
There was a problem hiding this comment.
The requirement says the filters sidebar should appear only when people are loaded. Here PeopleFilters is rendered whenever isLoading is false, even if errors.people or errors.loading is true or people is empty. Consider adding conditions (e.g., !errors.loading && !errors.people && people.length > 0) so filters are only shown when the data has successfully loaded.
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 correctly implements all of the critical filtering, sorting, and URL search-parameter behaviors required by the advanced people table task. The table uses the shared filterPeople and sortPeople utilities, encodes filters and sort state into URL search params via SearchLink, getSearchWith, and useProjectParams, and preserves search params when navigating within the People page (including the navbar People link and person selection). The Name filter updates and removes the query param appropriately and performs case-insensitive matching against name, mother, and father as required, while the Century filter supports multiple selections, an “All” option, and uses append/getAll for the centuries param. Sorting is correctly limited to Name, Sex, Born, and Died, with the three-step cycle (ascending → descending → disabled) wired through sort and order=desc, and Mother/Father headers are non-sortable. The sidebar filters appear only when people are successfully loaded, and error and loading states are handled without breaking functionality. As minor, non-blocking polish, you could simplify a few comments, make the sort-order flag naming more expressive, and ensure you’ve run npm test, but these do not affect task completion—overall this is a solid, requirement-compliant implementation.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
4 participants
DEMO LINK