Initial solution #1110
Initial solution #1110
Conversation
src/App.tsx
Outdated
| <Route path="/"> | ||
| <Route index element={<HomePage />} /> | ||
| <Route path="/home" element={<Navigate to="/" replace />} /> | ||
| <Route path="/people" element={<PeoplePage />}> | ||
| <Route index /> | ||
| <Route path=":personSlug?" /> | ||
| </Route> | ||
| <Route path="*" element={<PageNotFound />} /> |
src/components/Navbar/Navbar.tsx
Outdated
| className={({ isActive }) => { | ||
| return cn('navbar-item', { | ||
| 'has-background-grey-lighter': isActive, | ||
| }); | ||
| }} |
There was a problem hiding this comment.
| className={({ isActive }) => { | |
| return cn('navbar-item', { | |
| 'has-background-grey-lighter': isActive, | |
| }); | |
| }} | |
| className={({ isActive }) => | |
| cn('navbar-item', { 'has-background-grey-lighter': isActive }) | |
| } |
| <p className="panel-tabs" data-cy="SexFilter"> | ||
| <SearchLink | ||
| className={cn({ 'is-active': sex === null })} | ||
| params={{ sex: null }} |
There was a problem hiding this comment.
You can create an enum for sex values
| <div className="panel-block"> | ||
| <SearchLink | ||
| className={cn('button is-link is-outlined is-fullwidth', { | ||
| 'is-outlined': sex === null && !centuries.length && query === '', |
There was a problem hiding this comment.
Create a variable for these conditions checks
| export const PeoplePage = () => { | ||
| const { people, error, loading, filteredPeople } = usePeople(); | ||
|
|
||
| const succesfullyLoadedStatus = !!people.length && !loading; |
There was a problem hiding this comment.
succesfullyLoadedStatus is descriptive but a bit verbose. Consider renaming it to isLoaded for simplicity.
| className={ | ||
| personSlug === person.slug ? 'has-background-warning' : '' | ||
| } |
There was a problem hiding this comment.
it's better to use classnames in cases like that, it allows you to dynamically toggle classes based on state or props without manually concatenating strings, making the code more maintainable and easier to read.
| pathname: `/people/${person.slug}`, | ||
| search: searchParams.toString(), | ||
| }} | ||
| className={person.sex === 'f' ? 'has-text-danger' : ''} |
There was a problem hiding this comment.
it's better to use classnames in cases like that, it allows you to dynamically toggle classes based on state or props without manually concatenating strings, making the code more maintainable and easier to read.
| setLoading(true); | ||
| setError(false); | ||
|
|
||
| getPeople() | ||
| .then(getPeopleWithParents) | ||
| .then(setPeople) | ||
| .catch(() => { | ||
| setError(true); | ||
| }) | ||
| .finally(() => { | ||
| setLoading(false); | ||
| }); |
There was a problem hiding this comment.
It's better to move the logic inside the useEffect hook to a separate function, as it improves code readability, reusability, and makes testing easier. It also helps keep useEffect focused on side effects.
DEMO LINK