solution #1125
solution #1125
Conversation
There was a problem hiding this comment.
Great job on implementing the task requirements! 🎉 While there are some minor areas for improvement, such as simplifying routing logic and refining useEffect dependencies, these do not significantly 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! ✨
| <Route index element={<HomePage />} /> | ||
| <Route path=":home" element={<HomePage />} /> |
There was a problem hiding this comment.
There is a potential redundancy here. Both the index route and the :home route point to HomePage. If :home is intended to be a dynamic segment, consider clarifying its purpose or removing it if unnecessary.
src/components/PeopleFilters.tsx
Outdated
| }) => { | ||
| const query = searchParams.get('query') || ''; | ||
| const sex = searchParams.get('sex') || ''; | ||
| const centuries = searchParams.getAll('centuries') || []; |
There was a problem hiding this comment.
The getAll method for searchParams will return an array, so the fallback to an empty array is unnecessary. Consider removing || [].
| onCenturieFilter(); | ||
|
|
||
| setCurrentPeopleList(newPeopleList); | ||
| }, [sex, query, centuries]); |
There was a problem hiding this comment.
Ensure that setCurrentPeopleList is included in the dependency array of useEffect if it is not guaranteed to be stable across renders. This can prevent potential bugs related to stale closures.
|
|
||
| useEffect(() => { | ||
| setNewList(newPeopleList); | ||
| }, [peopleList]); |
There was a problem hiding this comment.
Consider adding newPeopleList to the dependency array of this useEffect to ensure it updates correctly when peopleList changes.
|
|
||
| useEffect(() => { | ||
| sortListFunc(); | ||
| }, [sort, order, newList]); |
There was a problem hiding this comment.
The newList dependency in this useEffect might cause unnecessary re-renders. Since sortListFunc updates newList, it might be better to remove newList from the dependencies to avoid potential infinite loops.
| if (home === 'home') { | ||
| return <Navigate to="/" replace />; | ||
| } |
There was a problem hiding this comment.
The check for home === 'home' and subsequent redirect to '/' seems unnecessary. If the home parameter is not required for any specific logic, consider removing this condition to simplify the component.
src/pages/PeoplePage.tsx
Outdated
|
|
||
| export const PeoplePage = () => { | ||
| const [people, setPeople] = useState<Person[]>([]); | ||
| const [currentPeopleList, setCurrentPeopleList] = useState<Person[]>(people); |
There was a problem hiding this comment.
Initializing currentPeopleList with people directly might lead to unexpected behavior since people is initially an empty array. Consider initializing currentPeopleList as an empty array or updating it in the useEffect after people is fetched.
src/components/PersonLink.tsx
Outdated
| <Link | ||
| to={`/people/${person.mother.slug}`} | ||
| className={classNames({ | ||
| 'has-text-danger': person.mother.sex === 'f', |
There was a problem hiding this comment.
Create a enum for 'f' and use it everywhere
src/pages/PeoplePage.tsx
Outdated
| <p data-cy="peopleLoadingError">Something went wrong</p> | ||
| )} | ||
|
|
||
| {!loading && people.length === 0 && ( |
There was a problem hiding this comment.
| {!loading && people.length === 0 && ( | |
| {!loading && !people.length && ( |
src/pages/PeoplePage.tsx
Outdated
| </p> | ||
| )} | ||
|
|
||
| {!loading && !error && people.length > 0 && ( |
There was a problem hiding this comment.
| {!loading && !error && people.length > 0 && ( | |
| {!loading && !error && !!people.length && ( |
https://Shevchuchka.github.io/react_people-table-advanced/