add task solution#1814
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation covers the core filtering and sorting functionality correctly, but two explicit task requirements remain violated:
1. Requirement #4 violation — The PeopleFilters component is rendered unconditionally in PeoplePage.tsx, but the task explicitly states the sidebar should appear "only when people are loaded." Move <PeopleFilters /> inside the conditional block that checks !isLoading && !error && people.length > 0.
2. Requirement #3 violation — The Navbar's "People" link uses standard <Link to="/people"> instead of <SearchLink to="/people">. This breaks search param preservation when clicking "People" from within the People page. Replace it with <SearchLink to="/people"> which will preserve existing search params.
Minor issue: There are linting/indentation errors in src/components/PeoplePage.tsx (lines 130, 132, 133). Run npm run lint -- --fix to auto-fix these.
Once these issues are addressed, the implementation should pass all requirements.
✨ 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
| @@ -1,4 +1,11 @@ | |||
| import { Link, useLocation } from 'react-router-dom'; | |||
There was a problem hiding this comment.
Missing implementation: The PeoplePage component with filtering/sorting logic is not included. Per task requirements, this must include URL search params for query, centuries, sort, and order parameters.
| @@ -1,4 +1,11 @@ | |||
| import { Link, useLocation } from 'react-router-dom'; | |||
There was a problem hiding this comment.
Missing implementation: The PeopleFilters component with NameFilter (query param) and CenturyFilter (centuries params using append/getAll) is not included.
| @@ -1,4 +1,11 @@ | |||
| import { Link, useLocation } from 'react-router-dom'; | |||
There was a problem hiding this comment.
Missing implementation: The PeopleTable component with 3-click sorting logic (ascending → descending → disabled) and sort/order URL params is not included.
| })} | ||
|
|
There was a problem hiding this comment.
Task requirement violation: The <PeopleFilters /> component at line 109 is always rendered, but requirement #3 states 'The sidebar with the filters should appear only when people are loaded.' Consider wrapping it with a condition like {!isLoading && !error && people.length > 0 && <PeopleFilters />} to ensure filters only appear after successful data load.
| <td> | ||
| <Link | ||
| to={{ | ||
| pathname: `/people/${person.slug}`, |
There was a problem hiding this comment.
The PeopleFilters component is rendered unconditionally regardless of loading state. According to requirement #3, the sidebar with filters should appear only when people are loaded. Consider adding a condition to check if data is loaded (e.g., !isLoading && people.length > 0) before rendering the filters sidebar.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The E2E test should not mark People nav link as active is failing because the People nav link is incorrectly being marked as active when on the home page. This is a functional issue in the navigation component. The Navbar's People link uses <SearchLink params={{}}> which may be causing the active state logic to incorrectly trigger. You need to ensure the People nav link is only marked as active when the user is actually on the People page (#/people), not on the home page (#/). Check the NavLink or active state logic in your navigation component and ensure it properly compares against the current URL/hash location.
✨ 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 request meets the required criteria for approval.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| {isPeopleActive ? ( | ||
| <SearchLink | ||
| aria-current="page" | ||
| className="navbar-item has-background-grey-lighter is-active" | ||
| params={{}} | ||
| > | ||
| People | ||
| </SearchLink> |
There was a problem hiding this comment.
The SearchLink with params={} creates a URL with trailing ? (e.g., /#/?), which may interfere with active state detection. Change to a standard Link component when isPeopleActive is true.
| > | ||
| People |
There was a problem hiding this comment.
Replace SearchLink with Link here to use an absolute path /people instead of relying on current search params, ensuring the link doesn't match when on the home page.
| .then(fetchedPeople => { | ||
| const peopleWithRelations = fetchedPeople.map(person => { | ||
| const mother = person.motherName | ||
| ? fetchedPeople.find(human => human.name === person.motherName) | ||
| : undefined; | ||
| const father = person.fatherName | ||
| ? fetchedPeople.find(human => human.name === person.fatherName) |
There was a problem hiding this comment.
Replace SearchLink with Link component here. Using SearchLink with empty params creates a URL with a trailing ? character (e.g., /#/?) which can interfere with pathname-based active state detection. A standard Link to="/people" is more reliable for the active navigation state.
2 participants
https://24o07.github.io/react_people-table-advanced/