react_people-table-advanced #1127
Conversation
There was a problem hiding this comment.
Great job on implementing the task requirements! 🎉 Your code meets the core functionality and constraints, effectively handling filtering and sorting with URL parameters. While there are some minor improvements suggested, such as optimizing the findPersonByName function and simplifying the useEffect logic, these do not critically impact the functionality. Keep up the good work, and consider the feedback for future enhancements! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/components/CenturyFilter.tsx
Outdated
| useEffect(() => { | ||
| if (selectCenturies) { | ||
| setSelectCenturies(selectCenturies); | ||
| } | ||
| }, [selectCenturies]); |
There was a problem hiding this comment.
The useEffect hook here is redundant and potentially problematic. It sets selectCenturies to itself whenever it changes, which can cause an infinite loop of updates. Consider removing this useEffect unless there's a specific reason for its presence that isn't clear from the current code.
src/components/PeoplePage.tsx
Outdated
| <p data-cy="noPeopleMessage">There are no people on the server</p> | ||
|
|
||
| <p>There are no people matching the current search criteria</p> | ||
| {isLoading && <Loader />} |
There was a problem hiding this comment.
The loading state is already handled at the beginning of the component with a return statement. Therefore, the check for isLoading here is redundant and can be removed to simplify the code.
| const findPersonByName = (name: string | undefined | null): Person | null => | ||
| name | ||
| ? people.find(p => p.name.toLowerCase() === name.toLowerCase()) || null | ||
| : null; |
There was a problem hiding this comment.
The findPersonByName function is called multiple times for each person in the table, which can be inefficient. Consider optimizing this by creating a map of names to Person objects outside of the map function to reduce redundant lookups.
src/components/PeopleTable.tsx
Outdated
| </td> | ||
| </tr> | ||
| {filteredPeople.map(person => ( | ||
| <tr |
There was a problem hiding this comment.
Consider using a React.Fragment (<>...</>) instead of an empty tag (<>) for consistency and readability.
| path: string, | ||
| ) => { | ||
| e.preventDefault(); | ||
| navigate(`${path}${location.search}`); |
There was a problem hiding this comment.
The location object is not imported or defined in this component. You should use searchParams directly instead of location.search to ensure the correct query string is appended to the navigation path.
src/components/SexFilter.tsx
Outdated
| className={classNames('is-active', { | ||
| 'is-active': filterBySex === 'all', | ||
| })} |
There was a problem hiding this comment.
The className logic for determining the active state is redundant. The classNames function is called with 'is-active' twice. You can simplify this by directly using filterBySex === 'all' without wrapping it in classNames.
src/components/SexFilter.tsx
Outdated
| className={classNames('is-active', { | ||
| 'is-active': filterBySex === 'm', | ||
| })} |
There was a problem hiding this comment.
Similar to the previous comment, the className logic for the 'Male' filter is redundant. Simplify by directly using filterBySex === 'm'.
src/components/SexFilter.tsx
Outdated
| className={classNames('is-active', { | ||
| 'is-active': filterBySex === 'f', | ||
| })} |
There was a problem hiding this comment.
Similarly, simplify the className logic for the 'Female' filter by directly using filterBySex === 'f'.
DEMO LINK