Solution #1177
Solution #1177
Conversation
src/components/PeoplePage.tsx
Outdated
| )} | ||
| {!isLoading && people.length !== 0 && ( | ||
| <PeopleTable people={sortPeople(filterPeople())} /> | ||
| )} | ||
| <p>There are no people matching the current search criteria</p> |
There was a problem hiding this comment.
The text should not always be visible
| } else { | ||
| updatedSortParams = { sort: sortBy }; | ||
| } | ||
|
|
There was a problem hiding this comment.
Thank you for your hard work on this project! 😊 There are a few critical issues that need to be addressed before we can approve the pull request. The main concerns are related to the to prop in the Link components, which should include both pathname and search properties to ensure proper navigation. Additionally, the nested routes under '/people' need restructuring to correctly render components. Please address these issues, and feel free to reach out if you need any assistance. Keep up the great work! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.
src/App.tsx
Outdated
| <Route path="/people"> | ||
| <Route index element={<PeoplePage />} /> | ||
| <Route path=":selectedUserSlug" element={<PeoplePage />} /> | ||
| </Route> |
There was a problem hiding this comment.
The nested routes under '/people' are not correctly structured. You should use the 'element' prop directly on the '/people' route if you want to render a component for this path. Alternatively, if you intend to have nested routes, ensure that the parent route has an 'element' prop that renders an 'Outlet' component for the child routes to render into.
| className={cN('button mr-1', { | ||
| 'is-info': searchParams.getAll('centuries').includes(century), | ||
| })} | ||
| to={getCenturiesFilterLink(century)} |
There was a problem hiding this comment.
The to prop in the Link component expects a string or an object with a pathname and search property. Ensure that getCenturiesFilterLink returns an object with a pathname or directly returns a string URL. Currently, it returns an object with only a search property, which might not be sufficient.
| return ( | ||
| <nav className="panel"> | ||
| <p className="panel-heading">Filters</p> | ||
|
|
||
| <p className="panel-tabs" data-cy="SexFilter"> | ||
| <a className="is-active" href="#/people"> | ||
| <Link className={getSexFilterClass(null)} to={getSexFilterLink(null)}> |
There was a problem hiding this comment.
The to prop in the Link component expects a string or an object with a pathname and search property. Ensure that getSexFilterLink returns an object with a pathname or directly returns a string URL. Currently, it returns an object with only a search property, which might not be sufficient.
| </a> | ||
| <a className="" href="#/people?sex=m"> | ||
| </Link> | ||
| <Link className={getSexFilterClass('m')} to={getSexFilterLink('m')}> |
There was a problem hiding this comment.
Similar to the previous comment, ensure that getSexFilterLink returns a valid to prop for the Link component.
| </a> | ||
| <a className="" href="#/people?sex=f"> | ||
| </Link> | ||
| <Link className={getSexFilterClass('f')} to={getSexFilterLink('f')}> |
There was a problem hiding this comment.
Ensure that getSexFilterLink returns a valid to prop for the Link component, as mentioned in previous comments.
| </span> | ||
| </th> | ||
|
|
||
| <th> | ||
| <span className="is-flex is-flex-wrap-nowrap"> | ||
| Sex | ||
| <a href="#/people?sort=sex"> | ||
| <Link to={getSortLink('sex')}> |
There was a problem hiding this comment.
Ensure that getSortLink returns a valid to prop for the Link component, as mentioned in previous comments.
| </span> | ||
| </th> | ||
|
|
||
| <th> | ||
| <span className="is-flex is-flex-wrap-nowrap"> | ||
| Born | ||
| <a href="#/people?sort=born&order=desc"> | ||
| <Link to={getSortLink('born')}> |
There was a problem hiding this comment.
Ensure that getSortLink returns a valid to prop for the Link component, as mentioned in previous comments.
| </span> | ||
| </th> | ||
|
|
||
| <th> | ||
| <span className="is-flex is-flex-wrap-nowrap"> | ||
| Died | ||
| <a href="#/people?sort=died"> | ||
| <Link to={getSortLink('died')}> |
There was a problem hiding this comment.
Ensure that getSortLink returns a valid to prop for the Link component, as mentioned in previous comments.
src/components/PersonLink.tsx
Outdated
|
|
||
| export const PersonLink: React.FC<Props> = ({ person }) => { | ||
| const { name, sex, slug } = person; | ||
| const curruntSearch = useLocation().search; |
There was a problem hiding this comment.
There is a typo in the variable name curruntSearch. It should be corrected to currentSearch for clarity and correctness.
| to={{ | ||
| search: getSearchWith(searchParams, params), | ||
| search: getSearchWith(params, searchParams), | ||
| }} |
There was a problem hiding this comment.
The to prop should include a pathname along with the search property to ensure proper navigation. Consider updating the getSearchWith function or the to prop to include a pathname.
There was a problem hiding this comment.
Great job on implementing the task requirements! 🎉 The code is well-structured and meets all the core functionality requirements. There are a couple of minor issues to address: correcting the typo in the variable name curruntSearch to currentSearch, and ensuring the "no people" message is conditionally rendered only when appropriate. Keep up the good work and happy coding! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/components/PeoplePage.tsx
Outdated
| {!isLoading && people.length !== 0 && ( | ||
| <PeopleTable people={sortPeople(filterPeople())} /> | ||
| )} | ||
| <p>There are no people matching the current search criteria</p> |
There was a problem hiding this comment.
The message 'There are no people matching the current search criteria' is always displayed, even when there are people matching the criteria. Consider conditionally rendering this message only when the filtered and sorted people array is empty.
src/components/PersonLink.tsx
Outdated
|
|
||
| export const PersonLink: React.FC<Props> = ({ person }) => { | ||
| const { name, sex, slug } = person; | ||
| const curruntSearch = useLocation().search; |
There was a problem hiding this comment.
There is a typo in the variable name curruntSearch. It should be corrected to currentSearch for clarity and consistency.
… name typo and refactor
There was a problem hiding this comment.
Great job on your implementation! 🎉 Your code meets the task requirements and constraints, and all necessary files are present. You've effectively addressed previous issues, such as correcting the typo in currentSearch and conditionally rendering the "no people" message. Your components are well-structured, and the logic for filtering, sorting, and managing URL search parameters is correctly implemented.
While there were no critical issues found, always remember to review comments and suggestions to further improve your code. This will help you enhance your skills and avoid similar issues in the future. Keep up the good work and continue learning from each experience! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
https://save-v.github.io/react_people-table-advanced/