Implement filtering and sorting with URL search params#1801
Implement filtering and sorting with URL search params#1801CyborgNinjaHat wants to merge 2 commits into
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The "People" link in the Navbar doesn't preserve search params when clicked. When a user applies filters/sorting on the People page and then clicks the "People" link in the Navbar, all their filter and sort selections are lost. Per the explicit requirement "Keep search params when navigating within the People page (when clicking the People link)", this link should preserve the current search params just like the PersonLink component does. To fix this, modify Navbar.tsx to append searchParams.toString() to the People link when the user is already on the People page. You can use the same pattern from PersonLink.tsx where it checks if it should preserve params. This is a straightforward fix but critical since it violates a named requirement from the task.
✨ 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
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The implementation has two issues that need to be addressed: First, the People NavLink in the Navbar navigates to /people without preserving current search params, which violates requirement #2 (Keep search params when navigating within the People page). The NavLink should use useSearchParams and append searchParams.toString() similar to how PersonLink handles this. Second, the CenturyFilter rebuilds the centuries array when toggling instead of using the URLSearchParams append method - at line 78 in PeopleFilters.tsx, the logic removes centuries from an array rather than properly handling the append behavior. These are functional issues that affect the expected behavior of preserving filter state during navigation and properly managing multiple century selections.
✨ 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
| setSearchParams(search); | ||
| }; |
There was a problem hiding this comment.
The People NavLink always navigates to /people without preserving search params. Per requirement #2, search params should be kept when navigating within the People page (when clicking the People link). Consider using useSearchParams to append current search params to the link.
| })} | ||
| params={{ | ||
| centuries: centuries.includes(century) | ||
| ? centuries.filter(num => century !== num) | ||
| : [...centuries, century], | ||
| }} |
There was a problem hiding this comment.
The century filter removes centuries from the array when clicked again (line 78). According to the requirements, it should use the append method. Consider using URLSearchParams append method instead of rebuilding the array to properly handle multiple centuries selection.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation successfully meets all requirements. The People page correctly preserves search params when navigating (Navbar uses useSearchParams + searchParams.toString(), PersonLink does the same), filters work properly (NameFilter uses query param with empty value removal, CenturyFilter uses getAll('centuries') with array handling via append), sorting implements the 3-state cycle correctly (ascending → descending → disabled), and the sidebar only displays when people are loaded (isLoaded && hasPeople condition). All URL state management uses getSearchWith utility which properly handles array values. The code is clean, well-structured, and demonstrates good separation of concerns. Great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
3 participants
DEMO LINK