Implement filtering and sorting features#1835
Conversation
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 requirements are implemented and working as specified: filters (sex, name, centuries) and tri‑state sorting are fully driven by URL search params, navigation preserves search params, and the filters sidebar only appears after data is loaded. The people list is correctly filtered by sex, multiple centuries using getAll('centuries'), and a case‑insensitive query across name, motherName, and fatherName, while sorting for name, sex, born, and died follows the required 3‑click cycle with sort and order parameters.
The remaining issues I see are non‑blocking: for example, the console.log in src/components/Navbar.tsx (reported by ESLint’s no-console rule) should be removed before final submission, and rerunning Prettier would help with a few minor formatting inconsistencies. You might also consider tightening types in SortLink (e.g., using a union type for the field prop) and simplifying some mapping logic in PeoplePage for clarity, but these are refinements, not correctness issues. Overall, you’ve met the task’s checklist requirements, and only small cleanups are left, which you can address as part of your usual lint/format pass.
✨ 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
| <Route path="/" element={<h1 className="title mt-6">Home Page</h1>} /> | ||
| <Route path="home" element={<Navigate to="/" replace />} /> | ||
| <Route path="people"> | ||
| <Route index element={<PeoplePage />} /> |
There was a problem hiding this comment.
Leaving console.log in production code is usually discouraged; consider removing it after you finish debugging to keep the console clean and avoid noise during automated checks.
|
|
||
| export const Navbar = () => { | ||
| const location = useLocation(); | ||
| console.log('location', location.pathname); |
There was a problem hiding this comment.
console.log is useful during development, but it’s better to remove or guard it before final submission to keep the console clean in production; consider removing it or using a debug logger instead.
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 it fully implements the required functionality and matches the checklist items: all filters and sorting are driven by URL search params, tri‑state sorting works correctly, and navigation within the People section preserves state between list and detail views. Your PeoplePage correctly applies sex, name, and century filters (including case‑insensitive name matching across name, motherName, and fatherName), and only shows the filters sidebar after data is loaded, which matches the spec. The sorting behavior via SortLink and PeopleTable is implemented as required for name, sex, born, and died, including the three‑step cycle (asc → desc → off) and proper removal of sort/order from the URL when sorting is disabled.
There are no critical blockers; the remaining points are minor improvements, such as tightening the field type in SortLink to a union of valid sort keys and optionally extracting some of the filtering/sorting logic into helper functions for readability. Also make sure you rely on your linter/Prettier and npm test to catch any style or hidden edge cases, but these are process-oriented rather than functional issues. Overall, your implementation is solid and aligned with the assignment requirements—nice work.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
DEMO LINK