-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
solution #1183
base: master
Are you sure you want to change the base?
solution #1183
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
import { | ||
HashRouter as Router, | ||
Routes, | ||
Route, | ||
Navigate, | ||
} from 'react-router-dom'; | ||
import { App } from './App'; | ||
import { HomePage } from './components/HomePage'; | ||
import { PeoplePage } from './components/PeoplePage'; | ||
|
||
export const Root = () => ( | ||
<Router> | ||
<Routes> | ||
<Route path="/" element={<App />}> | ||
<Route index element={<HomePage />} /> | ||
<Route path="home" element={<Navigate to="/" />} /> | ||
<Route path="people" element={<PeoplePage />}> | ||
<Route index /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
<Route path=":slug" /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
</Route> | ||
|
||
<Route path="*" element={<h1 className="title">Page not found</h1>} /> | ||
</Route> | ||
</Routes> | ||
</Router> | ||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
import React from 'react'; | ||
|
||
export const HomePage: React.FC = () => { | ||
return <h1 className="title">Home Page</h1>; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
import React from 'react'; | ||
|
||
export const NotFoundPage: React.FC = () => { | ||
return <h1 className="title">Page not found</h1>; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,18 +1,60 @@ | ||
export const PeopleFilters = () => { | ||
import React from 'react'; | ||
import { useSearchParams } from 'react-router-dom'; | ||
import classNames from 'classnames'; | ||
import { SearchLink } from './SearchLink'; | ||
|
||
const centuries: number[] = [16, 17, 18, 19, 20]; | ||
|
||
export const PeopleFilters: React.FC = () => { | ||
const [searchParams, setSearchParams] = useSearchParams(); | ||
const selectedCenturies = searchParams.getAll('centuries') || []; | ||
|
||
const handleQueryChange = (event: React.ChangeEvent<HTMLInputElement>) => { | ||
const params = new URLSearchParams(searchParams); | ||
|
||
params.set('query', event.target.value.trim().toLowerCase()); | ||
setSearchParams(params); | ||
}; | ||
|
||
const handleToggleCentury = (century: string) => { | ||
const updatedCenturies = selectedCenturies.includes(century) | ||
? selectedCenturies.filter(centuryYear => centuryYear !== century) | ||
: [...selectedCenturies, century]; | ||
|
||
const params = new URLSearchParams(searchParams); | ||
|
||
params.delete('centuries'); | ||
updatedCenturies.forEach(cent => params.append('centuries', cent)); | ||
setSearchParams(params); | ||
}; | ||
|
||
return ( | ||
<nav className="panel"> | ||
<p className="panel-heading">Filters</p> | ||
|
||
<p className="panel-tabs" data-cy="SexFilter"> | ||
<a className="is-active" href="#/people"> | ||
<SearchLink | ||
params={{ sex: null }} | ||
className={classNames({ 'is-active': !searchParams.get('sex') })} | ||
> | ||
All | ||
</a> | ||
<a className="" href="#/people?sex=m"> | ||
</SearchLink> | ||
<SearchLink | ||
params={{ sex: 'm' }} | ||
className={classNames({ | ||
'is-active': searchParams.get('sex') === 'm', | ||
})} | ||
> | ||
Male | ||
</a> | ||
<a className="" href="#/people?sex=f"> | ||
</SearchLink> | ||
<SearchLink | ||
params={{ sex: 'f' }} | ||
className={classNames({ | ||
'is-active': searchParams.get('sex') === 'f', | ||
})} | ||
> | ||
Female | ||
</a> | ||
</SearchLink> | ||
</p> | ||
|
||
<div className="panel-block"> | ||
|
@@ -22,6 +64,7 @@ export const PeopleFilters = () => { | |
type="search" | ||
className="input" | ||
placeholder="Search" | ||
onChange={handleQueryChange} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
/> | ||
|
||
<span className="icon is-left"> | ||
|
@@ -33,63 +76,44 @@ export const PeopleFilters = () => { | |
<div className="panel-block"> | ||
<div className="level is-flex-grow-1 is-mobile" data-cy="CenturyFilter"> | ||
<div className="level-left"> | ||
<a | ||
data-cy="century" | ||
className="button mr-1" | ||
href="#/people?centuries=16" | ||
> | ||
16 | ||
</a> | ||
|
||
<a | ||
data-cy="century" | ||
className="button mr-1 is-info" | ||
href="#/people?centuries=17" | ||
> | ||
17 | ||
</a> | ||
|
||
<a | ||
data-cy="century" | ||
className="button mr-1 is-info" | ||
href="#/people?centuries=18" | ||
> | ||
18 | ||
</a> | ||
|
||
<a | ||
data-cy="century" | ||
className="button mr-1 is-info" | ||
href="#/people?centuries=19" | ||
> | ||
19 | ||
</a> | ||
|
||
<a | ||
data-cy="century" | ||
className="button mr-1" | ||
href="#/people?centuries=20" | ||
> | ||
20 | ||
</a> | ||
{centuries.map(century => ( | ||
<button | ||
key={century} | ||
data-cy="century" | ||
className={classNames('button mr-1', { | ||
'is-info': selectedCenturies.includes(`${century}`), | ||
})} | ||
onClick={() => handleToggleCentury(`${century}`)} | ||
> | ||
{century} | ||
</button> | ||
))} | ||
</div> | ||
|
||
<div className="level-right ml-4"> | ||
<a | ||
<button | ||
data-cy="centuryALL" | ||
className="button is-success is-outlined" | ||
href="#/people" | ||
className={classNames('button is-success', { | ||
'is-outlined': selectedCenturies.length !== 0, | ||
})} | ||
onClick={() => setSearchParams(new URLSearchParams(searchParams))} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
> | ||
All | ||
</a> | ||
</button> | ||
</div> | ||
</div> | ||
</div> | ||
|
||
<div className="panel-block"> | ||
<a className="button is-link is-outlined is-fullwidth" href="#/people"> | ||
<SearchLink | ||
className="button is-link is-outlined is-fullwidth" | ||
params={{ | ||
centuries: [], | ||
sex: null, | ||
query: null, | ||
}} | ||
> | ||
Reset all filters | ||
</a> | ||
</SearchLink> | ||
</div> | ||
</nav> | ||
); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,29 +1,81 @@ | ||
import { useMemo, useEffect, useState } from 'react'; | ||
import { useSearchParams } from 'react-router-dom'; | ||
import { PeopleFilters } from './PeopleFilters'; | ||
import { Loader } from './Loader'; | ||
import { PeopleTable } from './PeopleTable'; | ||
import { getPeople } from '../api'; | ||
import { Person } from '../types'; | ||
import { getFilteredPeople } from '../utils/getFilteredPeople'; | ||
|
||
export const PeoplePage = () => { | ||
const [people, setPeople] = useState<Person[]>([]); | ||
const [loading, setLoading] = useState(false); | ||
const [error, setError] = useState(false); | ||
|
||
const [searchParams] = useSearchParams(); | ||
|
||
const filteredPeople = useMemo(() => { | ||
const filters = { | ||
sex: searchParams.get('sex'), | ||
name: searchParams.get('query'), | ||
centuries: searchParams.getAll('centuries'), | ||
}; | ||
|
||
return getFilteredPeople(people, filters); | ||
}, [searchParams, people]); | ||
|
||
useEffect(() => { | ||
setLoading(true); | ||
|
||
getPeople() | ||
.then(response => setPeople(response)) | ||
.catch(() => setError(true)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider providing more detailed error handling to inform the user about the nature of the error. This can help in debugging and improving user experience. |
||
.finally(() => setLoading(false)); | ||
}, []); | ||
|
||
return ( | ||
<> | ||
<h1 className="title">People Page</h1> | ||
|
||
<div className="block"> | ||
<div className="columns is-desktop is-flex-direction-row-reverse"> | ||
<div className="column is-7-tablet is-narrow-desktop"> | ||
<PeopleFilters /> | ||
</div> | ||
{!loading && !error && ( | ||
<div className="column is-7-tablet is-narrow-desktop"> | ||
<PeopleFilters /> | ||
</div> | ||
)} | ||
|
||
<div className="column"> | ||
<div className="box table-container"> | ||
<Loader /> | ||
|
||
<p data-cy="peopleLoadingError">Something went wrong</p> | ||
|
||
<p data-cy="noPeopleMessage">There are no people on the server</p> | ||
{loading && <Loader />} | ||
|
||
<p>There are no people matching the current search criteria</p> | ||
{error && ( | ||
<p data-cy="peopleLoadingError"> | ||
Something went wrong. | ||
<button | ||
onClick={() => window.location.reload()} | ||
className="button is-link ml-2" | ||
> | ||
Retry | ||
</button> | ||
</p> | ||
)} | ||
|
||
<PeopleTable /> | ||
{!loading && !error && ( | ||
<> | ||
{people.length === 0 ? ( | ||
<p data-cy="noPeopleMessage"> | ||
There are no people on the server | ||
</p> | ||
) : filteredPeople.length === 0 ? ( | ||
<p> | ||
There are no people matching the current search criteria | ||
</p> | ||
) : ( | ||
<PeopleTable people={filteredPeople} /> | ||
)} | ||
Comment on lines
+66
to
+76
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The conditional rendering for no data and no matching criteria is overlapping. Consider consolidating these conditions to avoid redundancy and ensure clarity. |
||
</> | ||
)} | ||
</div> | ||
</div> | ||
</div> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
<Route index />
inside thepeople
route does not specify an element. This will result in a route that does nothing when accessed. Consider providing a component or redirect for this index route.