Solution#1837
Conversation
mariiamad
commented
Jul 6, 2026
mariiamad
commented
- DEMO LINK
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 a couple of requirement-level issues that affect core functionality around URL search params and navigation. First, when selecting a person, PersonLink currently uses a plain Link to /people/${slug} without preserving the current search params; the checklist explicitly requires keeping filters/sort in the URL when choosing a person [CHECKLIST ITEM #4]. You should adjust PersonLink (e.g., by using SearchLink or including the current location.search in the to value) so that ?sex=...&query=...¢uries=...&sort=...&order=... remain intact when navigating between people. Second, the People navbar link also uses a plain NavLink to /people, which drops existing search params when returning from /people/:slug; this violates the requirement to keep search params when clicking the People link [CHECKLIST ITEM #5], so it should similarly preserve the current search string. Finally, the filters sidebar is rendered whenever !isLoading, even in error or empty-data states; the requirement says the sidebar should appear only when people are loaded, so consider guarding with !isLoading && !hasError && people.length > 0 around the filters. Aside from these points, your filtering and sorting logic, use of useSearchParams, and handling of query and centuries are well implemented, so once you fix these navigation and visibility issues your solution will be in great shape.
✨ 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
|
|
||
| const [searchParams, setSearchParams] = useSearchParams(); | ||
|
|
||
| const sex = searchParams.get('sex') || ''; |
There was a problem hiding this comment.
sex is declared as string | null in PeopleFilters props, but here it's always a string ('' fallback). While this still works (checked against 'm'/'f' and falsy otherwise), consider aligning the type or using null consistently for "no sex filter" if tests assert exact behavior.
| const resetAll = () => { | ||
| setSearchParams({}); |
There was a problem hiding this comment.
resetAll clears all search params (including sort and filters) with setSearchParams({}), which matches the "Reset all filters" requirement. Just ensure this is the intended behavior for sort as well, since it will also clear sort and order.
| const getSortParams = (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.
getSortParams implements the 3-click cycle (off → asc → desc → off) and relies on getSearchWith to remove params when values are null, satisfying the sort toggle requirements. No change needed here, but keep this behavior in sync with any tests around URL params.
| const sortedPeople = useMemo(() => { | ||
| const sorted = [...visiblePeople]; | ||
|
|
||
| switch (sort) { | ||
| case 'name': | ||
| sorted.sort((a, b) => a.name.localeCompare(b.name)); | ||
| break; | ||
|
|
||
| case 'sex': | ||
| sorted.sort((a, b) => a.sex.localeCompare(b.sex)); | ||
| break; | ||
|
|
||
| case 'born': | ||
| sorted.sort((a, b) => a.born - b.born); | ||
| break; | ||
|
|
||
| case 'died': | ||
| sorted.sort((a, b) => a.died - b.died); | ||
| break; | ||
| } | ||
|
|
||
| if (order === 'desc') { | ||
| sorted.reverse(); | ||
| } | ||
|
|
||
| return sorted; | ||
| }, [visiblePeople, sort, order]); |
There was a problem hiding this comment.
sort and order are used to choose which field to sort by and whether to reverse, and sortedPeople is based on visiblePeople. This correctly separates filtering from sorting and respects URL params. However, note that when no sort is set, the order is whatever the API returns; if tests require a specific default sort, you’d need to adjust here.
| {!isLoading && ( | ||
| <div className="block"> | ||
| <div className="columns is-desktop is-flex-direction-row-reverse"> | ||
| <div className="column is-7-tablet is-narrow-desktop"> | ||
| <PeopleFilters | ||
| sex={sex} | ||
| query={query} | ||
| handleQueryChange={handleQueryChange} | ||
| centuries={centuries} | ||
| toggleCentury={toggleCentury} | ||
| resetAll={resetAll} | ||
| /> | ||
| </div> | ||
|
|
||
| {visiblePeople.length === 0 ? ( | ||
| <div className="column"> | ||
| <div className="box table-container"> | ||
| <p data-cy="noPeopleMessage"> | ||
| There are no people matching the current search criteria | ||
| </p> | ||
| </div> | ||
| </div> | ||
| ) : ( | ||
| <div className="column"> | ||
| <div className="box table-container"> | ||
| <PeopleTable | ||
| people={sortedPeople} | ||
| getSortParams={getSortParams} | ||
| sort={sort} | ||
| order={order} | ||
| /> | ||
| </div> | ||
| </div> | ||
| )} | ||
| </div> | ||
| </div> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Filters (via PeopleFilters) are rendered whenever !isLoading, even if hasError is true or there are zero people. The requirement says the sidebar should appear only when people are loaded; you may want to guard this with a condition like !isLoading && !hasError && people.length > 0 to avoid showing filters in error/empty states.
| const [searchParams, setSearchParams] = useSearchParams(); | ||
|
|
||
| const sex = searchParams.get('sex') || ''; | ||
| const query = searchParams.get('query') ?? ''; |
There was a problem hiding this comment.
PersonLink uses Link to /people/${foundPerson.slug} without preserving current search params. This breaks checklist item #4 (keep search params when selecting a person); consider using SearchLink or useLocation to keep the existing search when navigating within the People page.
| getSortParams, | ||
| sort, | ||
| order, | ||
| }) => { |
There was a problem hiding this comment.
Here sex defaults to an empty string, so the actual runtime type is always string. The corresponding prop in PeopleFilters is typed as string | null, which is harmless but inconsistent; consider aligning the types (e.g., use just string) to avoid confusion about possible null values.
| 'fa-sort-down': sort === 'died' && order === 'desc', | ||
| })} | ||
| /> | ||
| </span> |
There was a problem hiding this comment.
getSortParams is declared with field: string, even though only the four known sort fields are valid. Since PeopleTable defines a SortField union, consider reusing that type here to tighten typing and catch accidental invalid fields at compile time.
|
|
||
| return ( | ||
| <Link | ||
| className={classNames({ 'has-text-danger': foundPerson.sex === 'f' })} |
There was a problem hiding this comment.
Checklist item #5 requires keeping search params when clicking the People link. Currently, this uses a plain NavLink to /people, which will drop existing search params (e.g. ?query=...&sort=...), so filters and sort are lost when returning to the People page. Consider using SearchLink here (or otherwise preserving useSearchParams) so that the current filters/sorting remain encoded in the URL when navigating back to /people.
| return ( | ||
| <Link | ||
| className={classNames({ 'has-text-danger': foundPerson.sex === 'f' })} | ||
| to={`/people/${foundPerson.slug}`} | ||
| > | ||
| {personName} | ||
| </Link> |
There was a problem hiding this comment.
PersonLink navigates to /people/${foundPerson.slug} using Link. This will drop current search params when moving between people, which conflicts slightly with the idea of keeping all filters in the URL when navigating within the People page. The checklist explicitly mentions preserving search params when selecting a person; to fully meet that, you should consider using SearchLink here (or constructing the to with the current search from useSearchParams) so filters/sorting remain in the URL when changing selected person.
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 core functional and technical requirements for the advanced People table are correctly implemented and behave as specified. Filters for sex, name, and century all live in URL search params, with query removed when the input is empty and centuries managed via getAll/append, and the filtering logic is case-insensitive on name, mother, and father. Sorting by name, sex, born, and died follows the required three-click cycle (asc → desc → off), and when sorting is disabled there are no sort or order params left in the URL. Navigation within the People page preserves search params both when selecting a person via PersonLink and when returning via the People navbar link, and the filters sidebar only appears once people are successfully loaded and non-empty. There are no critical blockers or broken behaviors evident in the provided code; remaining points (like tooling setup or test execution) are process-related rather than implementation issues. Overall, this is a solid, requirements-compliant implementation—nice work bringing all the pieces (filters, sorting, URL state, and navigation) together cleanly.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
