-
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
Develop #631
base: master
Are you sure you want to change the base?
Develop #631
Conversation
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.
Pls see the checklist on how sorting should work and fix your project becuase now it doesn't work
src/components/PeoplePage.tsx
Outdated
<PeopleFilters /> | ||
</div> | ||
|
||
{!isLoading && !hasError && !!people.length |
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.
pls consider moving long ternaries to separate consts with consistent names
src/components/PeopleFilters.tsx
Outdated
const [searchParams, setSearchParams] = useSearchParams(); | ||
const query = searchParams.get('query') || ''; | ||
const sex = searchParams.get('sex') || null; | ||
const centuryParams = searchParams.getAll('centuries') || []; |
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.
let's make a code little bit cleaner
const [searchParams, setSearchParams] = useSearchParams(); | |
const query = searchParams.get('query') || ''; | |
const sex = searchParams.get('sex') || null; | |
const centuryParams = searchParams.getAll('centuries') || []; | |
const [searchParams, setSearchParams] = useSearchParams(); | |
const query = searchParams.get('query') || ''; | |
const sex = searchParams.get('sex') || null; | |
const centuryParams = searchParams.getAll('centuries') || []; |
src/components/PeopleFilters.tsx
Outdated
</SearchLink> | ||
<SearchLink | ||
params={{ sex: 'm' }} | ||
className={cn({ 'is-active': sex === 'm' })} |
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.
m is a 'magic word'
Consider will be better if you create a enum for magic words and use it everywhere
src/pages/PeoplePage.tsx
Outdated
const [people, setPeople] = useState<Person[]>([]); | ||
const [isLoading, setIsLoading] = useState(false); | ||
const [hasError, setHasError] = useState(false); | ||
const [searchParams] = useSearchParams(); | ||
const query = searchParams.get('query') || ''; | ||
const sex = searchParams.get('sex') || null; | ||
const centuryParams = searchParams.getAll('centuries') || []; |
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.
const [people, setPeople] = useState<Person[]>([]); | |
const [isLoading, setIsLoading] = useState(false); | |
const [hasError, setHasError] = useState(false); | |
const [searchParams] = useSearchParams(); | |
const query = searchParams.get('query') || ''; | |
const sex = searchParams.get('sex') || null; | |
const centuryParams = searchParams.getAll('centuries') || []; | |
const [people, setPeople] = useState<Person[]>([]); | |
const [isLoading, setIsLoading] = useState(false); | |
const [hasError, setHasError] = useState(false); | |
const [searchParams] = useSearchParams(); | |
const query = searchParams.get('query') || ''; | |
const sex = searchParams.get('sex') || null; | |
const centuryParams = searchParams.getAll('centuries') || []; |
src/pages/PeoplePage.tsx
Outdated
setHasError(false); | ||
|
||
getPeople() | ||
.then(peopleData => setPeople(peopleData)) |
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.
.then(peopleData => setPeople(peopleData)) | |
.then(setPeople) |
src/pages/PeoplePage.tsx
Outdated
{!!people.length && !!filteredPeople.length && ( | ||
|
||
<PeopleTable people={filteredPeople} /> | ||
|
||
)} |
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.
{!!people.length && !!filteredPeople.length && ( | |
<PeopleTable people={filteredPeople} /> | |
)} | |
{!!people.length && !!filteredPeople.length && ( | |
<PeopleTable people={filteredPeople} /> | |
)} |
src/services/filterPeople.ts
Outdated
filteredPeople | ||
= filteredPeople.filter(person => person.name.toLowerCase() | ||
.includes(query)) | ||
|| filteredPeople.filter(person => person.motherName?.toLowerCase() | ||
.includes(normalizedQuery)) | ||
|| filteredPeople.filter(person => person.fatherName?.toLowerCase() | ||
.includes(normalizedQuery)); | ||
} |
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.
let's make a code readable
filteredPeople | |
= filteredPeople.filter(person => person.name.toLowerCase() | |
.includes(query)) | |
|| filteredPeople.filter(person => person.motherName?.toLowerCase() | |
.includes(normalizedQuery)) | |
|| filteredPeople.filter(person => person.fatherName?.toLowerCase() | |
.includes(normalizedQuery)); | |
} | |
filteredPeople | |
= filteredPeople.filter(person => { | |
return person.name.toLowerCase().includes(query); | |
}) | |
|| filteredPeople.filter(person => { | |
return person.motherName?.toLowerCase().includes(normalizedQuery); | |
}) | |
|| filteredPeople.filter(person => { | |
return person.fatherName?.toLowerCase().includes(normalizedQuery) | |
}); | |
} |
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.
Well done)
DEMO LINK