Solution #1160
Solution #1160
Conversation
| <NavLink | ||
| aria-current="page" | ||
| className="navbar-item has-background-grey-lighter" | ||
| href="#/people" | ||
| className={({ isActive }) => | ||
| cn('navbar-item', isTabActive({ isActive })) | ||
| } | ||
| to="people" | ||
| > | ||
| People | ||
| </a> | ||
| </NavLink> |
There was a problem hiding this comment.
here you forgot to add this. make sure to do this here and in all People link in table
"Keep search params when navigating within the People page (when selecting a person or clicking the People link)."
src/components/PeopleFilters.tsx
Outdated
| const query = searchParams.get('query') || ''; | ||
|
|
||
| const handleQuery = (event: React.ChangeEvent<HTMLInputElement>) => { | ||
| const params = new URLSearchParams(searchParams); | ||
|
|
||
| params.set('query', event.target.value); | ||
| setSearchParams(params); | ||
| }; |
There was a problem hiding this comment.
you need to make sure, that if query is empty, it should be deleted from searchParams
src/components/PeopleFilters.tsx
Outdated
| const handleGenderChange = ( | ||
| gender: Sex, | ||
| event: React.MouseEvent<HTMLAnchorElement, MouseEvent>, | ||
| ) => { | ||
| event.preventDefault(); | ||
| const params = new URLSearchParams(searchParams); | ||
|
|
||
| params.set('sex', gender); | ||
| setSearchParams(params); | ||
| }; |
There was a problem hiding this comment.
and for sex as well as for query - empty parameter should not be in search params
There was a problem hiding this comment.
Great job on your implementation! 🎉 Your code meets the task requirements and constraints effectively. While there are a few minor issues related to routing and sorting logic, they don't significantly impact the overall functionality. Keep up the good work, and consider addressing these minor points to further enhance your code. Happy coding! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| case 'born': | ||
| case 'died': | ||
| const result3 = filteredPeople.sort((person1, person2) => { | ||
| return person1.born - person2.born; |
There was a problem hiding this comment.
The sorting logic for 'born' and 'died' cases is currently identical, both sorting by 'born'. If you intend to sort by 'died' as well, you should adjust the logic to compare 'person1.died - person2.died' for the 'died' case.
| @@ -32,9 +112,14 @@ export const PeopleTable = () => { | |||
| <th> | |||
| <span className="is-flex is-flex-wrap-nowrap"> | |||
| Born | |||
| <a href="#/people?sort=born&order=desc"> | |||
| <a | |||
| href="#/people?sort=born&order=desc" | |||
There was a problem hiding this comment.
The href attribute in the 'Born' column header includes &order=desc, which might not be necessary since the handleSortByCategory function manages the order state. Consider removing it to avoid potential confusion.
| <Route path="people" element={<PeoplePage />}> | ||
| <Route path=":slug?" element={<PeoplePage />} /> |
There was a problem hiding this comment.
The nested route <Route path=":slug?" element={<PeoplePage />} /> inside the people route might cause the PeoplePage component to render twice when a slug is present. Consider reviewing the routing logic to ensure the component is rendered as intended.
DEMO LINK