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

implemented the ability to filter and sort people in the table #495

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

implemented the ability to filter and sort people in the table #495

wants to merge 4 commits into from

Conversation

OlhaMomot
Copy link

ivannaromanovych
ivannaromanovych suggested changes Oct 5, 2023
View reviewed changes
Copy link

@ivannaromanovych ivannaromanovych left a comment

Choose a reason for hiding this comment

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

Good job 🔥, but reverse sorting is not working
Left comments

src/components/Navbar.tsx Outdated
Comment on lines 4 to 7
const getLinkClass = ({ isActive }: { isActive: boolean }) => classNames(
'navbar-item',
{ 'has-background-grey-lighter': isActive },
);
Copy link

Choose a reason for hiding this comment

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

You can move this to utils

src/utils/constants.tsx Outdated
Comment on lines 25 to 28
export const SEARCH_FILTER = {
All: null,
Male: 'm',
Female: 'f',
Copy link

Choose a reason for hiding this comment

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

This search filter reffers only to sex, you can be more specific with name of constant

src/components/PeopleTable.tsx Outdated
case 'died':
return a[sort] - b[sort];
default:
return 1;
Copy link

Choose a reason for hiding this comment

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

Maybe it would be better to return 0, so the array stays unchanged in default case

src/components/PeopleTable.tsx Outdated
</td>
</tr>
if (currentColumnTitle === sort && !order) {
return { sort: null, order: 'desc' };
Copy link

Choose a reason for hiding this comment

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

Why do you delete sort option after second click, the reverse sorting is not working

Copy link
Author

Choose a reason for hiding this comment

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

By mistake, it was supposed to be just 'sort'🙈 Thanks!

roman-mirzoian
roman-mirzoian suggested changes Oct 5, 2023
View reviewed changes
Copy link

@roman-mirzoian roman-mirzoian left a comment

Choose a reason for hiding this comment

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

Good job, but there is quite a bit of work to be done.

src/components/PeopleFilters.tsx Outdated

const newQuery = e.target.value;

if (newQuery === '') {
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 (newQuery === '') {
if (!newQuery) {

src/components/PeopleFilters.tsx Outdated
<a className="is-active" href="#/people">All</a>
<a className="" href="#/people?sex=m">Male</a>
<a className="" href="#/people?sex=f">Female</a>
{Object.keys(SEX_FILTER).map(
Copy link

Choose a reason for hiding this comment

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

Use Object.entries

src/components/PeopleFilters.tsx Outdated
}}
data-cy="century"
className={classNames(
'button mr-1',
Copy link

Choose a reason for hiding this comment

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

Suggested change
'button mr-1',
'button', 'mr-1',

src/utils/constants.tsx Outdated
Female: 'f',
};

export const NO_PARENT_NAME = '-';
Copy link

Choose a reason for hiding this comment

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

Suggested change
export const NO_PARENT_NAME = '-';
export const NOT_SET_VALUE = '-';

src/utils/constants.tsx Outdated
Female: 'f',
};

export const NO_PARENT_NAME = '-';
Copy link

Choose a reason for hiding this comment

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

Suggested change
export const NO_PARENT_NAME = '-';
export const EMPTY_VALUE = '-';

src/utils/getPreparedPeople.tsx Outdated
Comment on lines 11 to 13
const findPersonByName = (personName: string | null) => {
return people.find(({ name }) => name === personName);
};
Copy link

Choose a reason for hiding this comment

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

You can move this to utils

src/utils/getPreparedPeople.tsx Outdated
|| person.fatherName?.toLowerCase().includes(query)
);

const bornCentury = (Math.ceil(person.born / 100)).toString();
Copy link

Choose a reason for hiding this comment

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

Avoid using "magic values" (100)

roman-mirzoian
roman-mirzoian approved these changes Oct 6, 2023
View reviewed changes
Copy link

@roman-mirzoian roman-mirzoian left a comment

Choose a reason for hiding this comment

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

Well done 👍

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

@roman-mirzoian roman-mirzoian roman-mirzoian approved these changes

@ivannaromanovych ivannaromanovych Awaiting requested review from ivannaromanovych

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

Successfully merging this pull request may close these issues.
3 participants
@OlhaMomot @ivannaromanovych @roman-mirzoian