Skip to content
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.

Sign up for GitHub

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

Jump to bottom

first solution #528

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

first solution #528

wants to merge 2 commits into from

Conversation

vensecue
Copy link

@vensecue vensecue commented Oct 6, 2023

choeqq
choeqq suggested changes Oct 9, 2023
View reviewed changes
src/components/PeoplePage.tsx Outdated
const [loadingError, setLoadingError] = useState(false);

const fetchData = () => {
fetch('https://mate-academy.github.io/react_people-table/api/people.json')
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use getPeople function provided

nataliaklonowska
nataliaklonowska suggested changes Oct 11, 2023
View reviewed changes
Copy link

@nataliaklonowska nataliaklonowska left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Organize your code to minimize duplication by using mapping where indicated.

src/components/PeopleFilters.tsx
Comment on lines +34 to +54
<SearchLink
className={cn({ 'is-active': selectedGender === null })}
params={{ sex: null }}
>
All

</SearchLink>
<SearchLink
className={cn({ 'is-active': selectedGender === 'm' })}
params={{ sex: 'm' }}
>
Male

</SearchLink>
<SearchLink
className={cn({ 'is-active': selectedGender === 'f' })}
params={{ sex: 'f' }}
>
Female

</SearchLink>
Copy link

@nataliaklonowska nataliaklonowska Oct 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Try to avoid repetition of the SearchLink component - you can use map method to generate links dynamically. Same for SearchLink with centuries - you can create array with possible centuries.
  • To make code more descriptive, it's a good practice to create an enum or variables for 'm', 'f', and null.
  • delete blank lines to improve code readability.

src/components/PeopleFilters.tsx
};

const handleQueryChange = (newValue: string) => {
if (newValue === '') {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (newValue === '') {
if (!newValue) {

src/components/PeoplePage.tsx

<p>There are no people matching the current search criteria</p>
{people?.length === 0 && (
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{people?.length === 0 && (
{!people?.length && (

src/components/PeopleTable.tsx
</table>
return (
<>
{filteredPeople.length === 0
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{filteredPeople.length === 0
{!filteredPeople.length

src/components/PeopleTable.tsx
Comment on lines +20 to +55
switch (sort) {
case 'born':
if (sortDirection === 'desc') {
filteredPeople.sort((a, b) => b.born - a.born);
} else {
filteredPeople.sort((a, b) => a.born - b.born);
}

break;
case 'died':
if (sortDirection === 'desc') {
filteredPeople.sort((a, b) => b.died - a.died);
} else {
filteredPeople.sort((a, b) => a.died - b.died);
}

break;
case 'name':
if (sortDirection === 'desc') {
filteredPeople.sort((a, b) => b.name.localeCompare(a.name));
} else {
filteredPeople.sort((a, b) => a.name.localeCompare(b.name));
}

break;
case 'sex':
if (sortDirection === 'desc') {
filteredPeople.sort((a, b) => b.sex.localeCompare(a.sex));
} else {
filteredPeople.sort((a, b) => a.sex.localeCompare(b.sex));
}

break;
case null:
default:
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • In order to simplify the code, you can consider reorganizing the switch statement to group similar sorting cases together (name and sex) (born and died)
  • delete blank lines before break

src/components/PeopleTable.tsx
Comment on lines +126 to +196
<th>
<span className="is-flex is-flex-wrap-nowrap">
Name
<a onClick={() => setSortParams('name')}>
<span className="icon">
<i className={cn({
'fas fa-sort': sortBy !== 'name',
'fas fa-sort-up': sortBy === 'name'
&& sortDirection !== 'desc',
'fas fa-sort-down': sortBy === 'name'
&& sortDirection === 'desc',
})}
/>
</span>
</a>
</span>
</th>

<th>
<span className="is-flex is-flex-wrap-nowrap">
Sex
<a onClick={() => setSortParams('sex')}>
<span className="icon">
<i className={cn({
'fas fa-sort': sortBy !== 'sex',
'fas fa-sort-up': sortBy === 'sex'
&& sortDirection !== 'desc',
'fas fa-sort-down': sortBy === 'sex'
&& sortDirection === 'desc',
})}
/>
</span>
</a>
</span>
</th>

<th>
<span className="is-flex is-flex-wrap-nowrap">
Born
<a onClick={() => setSortParams('born')}>
<span className="icon">
<i className={cn({
'fas fa-sort': sortBy !== 'born',
'fas fa-sort-up': sortBy === 'born'
&& sortDirection !== 'desc',
'fas fa-sort-down': sortBy === 'born'
&& sortDirection === 'desc',
})}
/>
</span>
</a>
</span>
</th>

<th>
<span className="is-flex is-flex-wrap-nowrap">
Died
<a onClick={() => setSortParams('died')}>
<span className="icon">
<i className={cn({
'fas fa-sort': sortBy !== 'died',
'fas fa-sort-up': sortBy === 'died'
&& sortDirection !== 'desc',
'fas fa-sort-down': sortBy === 'died'
&& sortDirection === 'desc',
})}
/>
</span>
</a>
</span>
</th>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can utilize mapping for th to iterate through possible sortings

src/components/PeopleTable.tsx
Comment on lines +204 to +206
{filteredPeople.map((person) => (

<tr
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{filteredPeople.map((person) => (
<tr
{filteredPeople.map((person) => (
<tr

src/components/PeopleTable.tsx
Comment on lines +236 to +238
: person.motherName || '-'}

</td>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
: person.motherName || '-'}
</td>
: person.motherName || '-'}
</td>

choeqq
choeqq suggested changes Oct 12, 2023
View reviewed changes
Copy link

@choeqq choeqq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting for any upadtes @vensecue 👀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nataliaklonowska nataliaklonowska nataliaklonowska requested changes

@choeqq choeqq choeqq requested changes

@arturwisnia arturwisnia arturwisnia approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vensecue @choeqq @arturwisnia @nataliaklonowska