add task solution#1799
Conversation
CyborgNinjaHat
left a comment
CyborgNinjaHat
left a comment
There was a problem hiding this comment.
Great job! However, I can't approve your pull request - the table is not sortable by columns, and that's a key requirement of the task. Fix that, and you're good to go.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
There are critical bugs that block task completion. First, typos in the sorting logic in PeoplePage.tsx — comparing a.born with a.born and a.died with a.died instead of comparing with the second parameter b. This breaks sorting by birth and death years entirely. Second, the sex filter is not connected to PeoplePage's filtering logic — the sex search param is read but never used in the filter condition. Third, the PeopleFilters component is rendered unconditionally in App.tsx (line 135), but requirement #3 specifies filters should only appear when people are loaded — this needs conditional rendering based on the loading state. Please fix these three issues before resubmission.
✨ 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
|
|
||
| case 'born': |
There was a problem hiding this comment.
Typo: comparing a.born with itself always returns 0. Should be a.born - b.born. This breaks sorting by birth year.
|
|
||
| case 'died': |
There was a problem hiding this comment.
Typo: comparing a.died with itself always returns 0. Should be a.died - b.died. This breaks sorting by death year.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
There's a critical bug in src/components/PeopleFilters.tsx where URL construction is malformed. When params.toString() returns an empty string, the current code returns ? instead of ., which creates invalid URLs with double question marks like ?query=mae?sort=name&order=asc. The fix should check if the query string is empty and return . for the current path instead:
✨ 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
| params.delete('query'); | ||
| } | ||
|
|
||
| setSearchParams(params); | ||
| }; | ||
|
|
||
| const toggleCentury = (value: number) => { |
There was a problem hiding this comment.
Critical bug: When params.toString() is empty, this returns ? creating malformed URLs like ?sort=name&order=asc or ?query=mae?sort=name (double ?). Should return . when params are empty to avoid this issue.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This is an educational approval to help you progress while learning. Your core implementation is strong - the filter and sort logic using URL search params is well done in PeoplePage.tsx, PeopleFilters.tsx, and PeopleTable.tsx. However, there's one critical bug that needs fixing: PersonLink.tsx uses a hardcoded path /people/${person.slug} without preserving current search params. When a user clicks a person's name, they lose all their filters and sorting settings, which violates checklist item #3. To fix this, use the SearchLink component (import it from ../SearchLink) or get the current search params with useSearchParams() and append them to your link. For example: <SearchLink params={{ query: search }} to={...}>. This is a common pattern in React apps and mastering it will serve you well. The good news is fixing one file (PersonLink.tsx) will resolve the issue across all the affected files since they all use this component.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| } | ||
|
|
||
| setSearchParams(params); | ||
| }; |
There was a problem hiding this comment.
This navigation doesn't preserve search params. Per requirement #2, clicking on a person should keep the current filters/sort active. Use the SearchLink component or construct a link with the current search params included.
| classNames('navbar-item', { | ||
| 'has-background-grey-lighter': isActive, | ||
| }) | ||
| } |
There was a problem hiding this comment.
The link uses a hardcoded path /people/${person.slug} without preserving search params. When navigating to a person's details, the user's active filters and sort settings are lost. Use the SearchLink component or include the current search params in the URL to preserve the filter/sort state as required by checklist item #3.
| <Navbar /> | ||
|
|
||
| <div className="section"> | ||
| <div className="container"> |
There was a problem hiding this comment.
This link navigates to /people/${person.slug} without preserving search params. According to checklist item #3, search params must be preserved when navigating within the People page (selecting a person). Use SearchLink component with preserved params instead.
|
|
||
| export const PeopleTable = ({ people = [], slug }: PeopleTableProps) => { | ||
| const [searchParams] = useSearchParams(); | ||
|
|
There was a problem hiding this comment.
This link navigates to /people/${person.slug} without preserving search params. According to checklist item #3, search params must be preserved when navigating within the People page. Use the SearchLink component with preserved search params, or construct the URL to include the current search string from useSearchParams.
| const isFemale = person.sex === 'f'; | ||
|
|
||
| return ( | ||
| <Link |
There was a problem hiding this comment.
This link navigates to /people/${person.slug} without preserving search params. According to requirement #3, search params must be preserved when navigating within the People page (selecting a person). Use useSearchParams to append the current search string, or refactor to use the SearchLink component that already exists in the codebase.
4 participants
DEMO LINK