solution#1800
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The implementation has several critical issues that must be fixed before approval:
1. Missing Sorting Implementation (CRITICAL): The PeopleTable.tsx correctly manages URL sort params with getSortSearch(), but the actual sorting of the people array is never implemented. The table renders visiblePeople in its original order regardless of sort settings.
2. Incomplete Query Filter (CRITICAL): Requirement #4 explicitly states to filter by name, motherName, OR fatherName (case insensitive), but PeoplePage.tsx line 29 only checks person.name. Add conditions to also check person.motherName and person.fatherName.
3. Search Params Not Preserved (CRITICAL): Requirement #2 states to keep search params when navigating within the People page. In Navbar.tsx, the People link uses <Link to="/people"> instead of <SearchLink>, which resets all filters. Similarly, PersonTable.tsx line 24-25 uses plain <Link> for person name navigation.
4. Century Calculation Uses Death Year (MEDIUM): Line 27 calculates diedCentury = Math.ceil(person.died / 100). Centuries should be based on birth year for historical accuracy.
Please fix these issues, particularly the sorting functionality which is a core feature currently missing entirely.
✨ 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
| </Link> | ||
|
|
||
| <a | ||
| <Link |
There was a problem hiding this comment.
The People link should use SearchLink instead of Link to preserve search params when navigating. This is required by task requirement #2: 'Keep search params when navigating within the People page (when selecting a person or clicking the People link).'
| const sex = searchParams.get('sex') || null; | ||
| const centuries = searchParams.getAll('centuries') || null; | ||
| const diedCentury = Math.ceil(person.died / 100); | ||
|
|
There was a problem hiding this comment.
The query filter only checks person.name, but requirement #4 states: 'show only people with the name, motherName or fatherName that match the query case insensitive'. The condition should also check person.motherName and person.fatherName.
| </Link> | ||
|
|
There was a problem hiding this comment.
The sorting logic updates URL params correctly, but the actual people array is never sorted before rendering. The people prop is rendered as-is without any sorting applied. You need to implement sorting logic that reads sort and order from searchParams and sorts the array accordingly before passing it to PersonTable.
| order: 'desc', | ||
| }); | ||
| } |
There was a problem hiding this comment.
The query filter only checks person.name, but requirement #4 states: 'show only people with the name, motherName or fatherName that match the query'. Add conditions to also check person.motherName and person.fatherName (with null checks since they can be null).
| } | ||
|
|
||
| if (order !== 'desc') { | ||
| return getSearchWith(searchParams, { |
There was a problem hiding this comment.
The century calculation uses death year (person.died) which may cause incorrect filtering. Consider using birth year instead, as centuries are typically associated with when a person was born.
| })} | ||
| > | ||
| <td> | ||
| <Link |
There was a problem hiding this comment.
The person name link doesn't preserve search params when navigating. According to requirement #2, clicking a person should keep current filters/sort. Consider using useSearchParams to include current search params in the link, similar to how PeopleFilters handles this.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation is approved. All critical issues from previous reviews have been successfully resolved: the 3-state sorting cycle now properly sorts the data, name filtering checks name, motherName, and fatherName fields, century calculation uses the birth year, and URL search params are correctly preserved throughout navigation. The codebase is clean and functional with no blockers remaining. Great work on addressing all the feedback!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
DEMO LINK