haha#1836
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because one core requirement is still not met: the “Reset all filters” control does not fully reset the filter state. Currently, in src/components/PeoplePage.tsx (the reset logic lives there in your PeopleFilters usage), the reset link clears query, centuries, sort, and order, but it leaves the sex search param intact, so the SexFilter remains active. This violates the requirement that the reset action must return the user to an entirely unfiltered state, which is a functional, testable behavior rather than a cosmetic detail.
To fix this, update the reset link’s params object so it also includes sex: null, for example:
- In the reset
SearchLink(or equivalent), change something like:{ query: null, centuries: null, sort: null, order: null }- to:
{ query: null, centuries: null, sort: null, order: null, sex: null }.
Once you make this change, the reset button will clear all filters (sex, name, centuries, and sorting), matching the task requirements and expected behavior. Apart from that issue, your handling of URL search params, filters, sorting, and navigation looks solid and well-structured—this is a small but important fix to complete the solution.
✨ 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
|
|
||
| <p data-cy="noPeopleMessage">There are no people on the server</p> | ||
| {!loading && !error && people.length === 0 && ( | ||
| <p data-cy="noPeopleMessage">There are no people on the server</p> |
There was a problem hiding this comment.
The "Reset all filters" link currently removes query, centuries, sort, and order, but it leaves the sex filter untouched. To fully match the requirement to reset all filters (checklist items #1, #3, and #30 implicitly), include sex: null in the params here so the SexFilter is also cleared when this button is clicked.
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 and URL-based filtering/sorting. All filters and sort options (query, sex, centuries, sort, order) are correctly driven by URL search params, and these params are preserved when navigating within the People section (via the navbar, person links, and parent links). The filters behave as specified: the name filter updates/removes query and matches name/mother/father case-insensitively, the century filter supports multiple selections and an “All” reset, and the sidebar only appears when people are successfully loaded. Sorting works with the required three-step cycle (ascending → descending → off) for name, sex, born, and died, and sort/order are correctly removed when sorting is disabled. The “Reset all filters” control now resets every filter-related parameter, including sex, to return the view to a fully unfiltered state.
The remaining points are minor: there are a few eslint/prettier disables and some duplicated search param handling that could be refactored into small helpers for cleanliness, but these are not blockers. Overall, the behavior is consistent, shareable via URLs, and aligned with the task description—well done bringing everything together so cleanly.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
DEMO LINK