implement the ability to filter and sort people in the table #516

KatOlista · 2023-10-05T12:15:44Z

BudnikOleksii

Looks great 👍
Left some suggestions, check them

BudnikOleksii · 2023-10-06T16:21:52Z

src/api.ts

+export const NOT_SET_VALUE = '-';
+
+export enum ColumnNames {
+  Name = 'Name',
+  Sex = 'Sex',
+  Born = 'Born',
+  Died = 'Died',
+}
+
+export enum PersonSex {
+  All = '',
+  Male = 'm',
+  Female = 'f',
+}


Move constants to the constants file/folder and enums/types to the types folder

BudnikOleksii · 2023-10-06T16:22:27Z

src/api.ts

+export type FilterType = {
+  query: string | '';
+  centuries: string[];
+  sex: string | '';
+};


Suggested change

export type FilterType = {

query: string | '';

centuries: string[];

sex: string | '';

};

export type FilterType = {

query: string;

centuries: string[];

sex: string;

};

An empty string is still a string

BudnikOleksii · 2023-10-06T16:23:30Z

src/api.ts

+function getParent(people: Person[], parentName: string) {
+  return people.find(({ name }) => name === parentName);
+}
+
+export function addParent(people: Person[]) {
+  return people.map(person => {


Move all helpers to the helpers or utils folder.

BudnikOleksii · 2023-10-06T16:27:58Z

src/api.ts

+    let mother;
+    let father;
+
+    if (person.motherName) {
+      mother = getParent(people, person.motherName);
+    }
+
+    if (person.fatherName) {
+      father = getParent(people, person.fatherName);
+    }
+
+    return {
+      ...person,
+      mother,
+      father,
+    };


Suggested change

let mother;

let father;

if (person.motherName) {

mother = getParent(people, person.motherName);

}

if (person.fatherName) {

father = getParent(people, person.fatherName);

}

return {

...person,

mother,

father,

};

const { motherName, fatherName } = person;

return {

...person,

mother: motherName ? getParent(people, motherName) : motherName,

father: getParent(people, fatherName), // or like this, and if no fatherName just return from the getParent function

};

BudnikOleksii · 2023-10-06T16:28:35Z

src/api.ts

+      switch (sortParam) {
+        case ('name'): {


sortParam should be enum

BudnikOleksii · 2023-10-06T16:30:19Z

src/api.ts

+        case ('born'): {
+          return a[sortParam] - (b[sortParam]);
+        }
+
+        case ('died'): {
+          return a[sortParam] - (b[sortParam]);
+        }


Suggested change

case ('born'): {

return a[sortParam] - (b[sortParam]);

}

case ('died'): {

return a[sortParam] - (b[sortParam]);

}

case (SortParam.born):

case (SortParam.died): {

return a[sortParam] - (b[sortParam]);

}

You can combine cases if they do a similar job

BudnikOleksii · 2023-10-06T16:33:01Z

src/api.ts

+  return people
+    .filter(person => {
+      if (filterOption.sex) {
+        return person.sex === filterOption.sex;
+      }
+
+      return person;
+    })
+    .filter(person => {


You iterate through an array of people every time even some of filterOption is not set

BudnikOleksii · 2023-10-06T16:41:04Z

src/components/PeopleTable/PeopleTable.tsx

+  if (order === 'desc') {
+    filteredPeople.reverse();
+  }


'desc' looks like a magic number, and reverse mutate your array

but I used reverse to the array copy, not to the original array. Is it a problem in this case?

BudnikOleksii · 2023-10-06T16:42:03Z

src/components/PersonLink/PersonLink.tsx

+enum Sex {
+  FEMALE = 'f',
+  MALE = 'm',
+}


enums should be stored in types folder/file

…r function, create vars for 'magic values'

Moroz-Dmytro

I have person with name "Emma" in table, but I see empty state

When I have empty state, table header should not be displayed

When I click on "People" -> my search params dissapear

When I open "People" page I see Filter panel for a second

Moroz-Dmytro · 2023-10-09T07:13:09Z

src/Root.tsx

+        <Route path="people" element={<PeoplePage />} />
+        <Route path="people/:slug?" element={<PeoplePage />} />


Suggested change

<Route path="people" element={<PeoplePage />} />

<Route path="people/:slug?" element={<PeoplePage />} />

<Route path="people">

<Route path=":slug?" element={<PeoplePage />} />

</Route>

Moroz-Dmytro · 2023-10-09T07:15:27Z

src/api.ts

+  });
+}
+
+export function sortPeople(people: Person[], sortParam: SortParam | string) {


Suggested change

export function sortPeople(people: Person[], sortParam: SortParam | string) {

export function getSortedPeople(people: Person[], sortParam: SortParam | string) {

Moroz-Dmytro · 2023-10-09T07:15:44Z

src/api.ts

+  return [...people];
+}
+
+export function filterPeople(


Suggested change

export function filterPeople(

export function getFilteredPeople(

Moroz-Dmytro · 2023-10-09T07:16:03Z

src/api.ts

+      .filter(person => person.name.includes(filterOption.query)
+    || person.motherName?.includes(filterOption.query)
+    || person.fatherName?.includes(filterOption.query));


Moroz-Dmytro · 2023-10-09T07:16:52Z

src/components/ColumnNameItem/ColumnNameItem.tsx

+            {lowerCasedValue !== sort && (<i className="fas fa-sort" />)}
+            {lowerCasedValue === sort && !order && (
+              <i className="fas fa-sort-up" />
+            )}
+            {order && lowerCasedValue === sort && (


DRY, create variable for repeatable part of code

Moroz-Dmytro · 2023-10-09T07:18:20Z

src/components/PeopleFilters/PeopleFilters.tsx

+    if (!event.target.value) {
+      setSearchParams(getSearchWith(searchParams,
+        { query: null }));
+    } else {
+      setSearchParams(getSearchWith(searchParams,
+        { query: event.target.value }));
+    }


Suggested change

if (!event.target.value) {

setSearchParams(getSearchWith(searchParams,

{ query: null }));

} else {

setSearchParams(getSearchWith(searchParams,

{ query: event.target.value }));

}

setSearchParams(

getSearchWith(searchParams, {

query: event.target.value || null

})

);

Moroz-Dmytro · 2023-10-09T07:19:21Z

src/components/PeopleFilters/PeopleFilters.tsx

+        <a
+          className="button is-link is-outlined is-fullwidth"
+          href="#/people"
+        >
+          Reset all filters
+        </a>


Use SearchLink here

roman-mirzoian

Good job, but pay attention to the comments.

roman-mirzoian · 2023-10-09T21:40:37Z

src/components/ColumnNameItem/ColumnNameItem.tsx

+  const getSortParams = (sortOption: string) => {
+    if (sortOption !== sort) {
+      return { sort: sortOption, order: null };
+    }
+
+    if (sort === sortOption && !order) {
+      return { sort: sortOption, order: 'desc' };
+    }
+
+    return { sort: null, order: null };
+  };


It is better to define this function outside the component so that it is not created every time a new render is performed.

roman-mirzoian · 2023-10-09T21:41:20Z

src/api.ts

+export function addParent(people: Person[]) {
+  return people.map(person => {
+    const { motherName, fatherName } = person;
+
+    return {
+      ...person,
+      mother: motherName ? getParent(people, motherName) : undefined,
+      father: fatherName ? getParent(people, fatherName) : undefined,
+    };
+  });
+}
+
+export function getSortedPeople(
+  people: Person[],
+  sortParam: SortParam | string,
+) {
+  if (sortParam) {
+    return [...people].sort((a, b) => {
+      switch (sortParam) {
+        case (SortParam.Name):
+        case (SortParam.Sex): {
+          return a[sortParam].localeCompare(b[sortParam]);
+        }
+
+        case (SortParam.Born):
+        case (SortParam.Died): {
+          return a[sortParam] - (b[sortParam]);
+        }
+
+        default: {
+          return 0;
+        }
+      }
+    });
+  }
+
+  return [...people];
+}
+
+const getFilteredPeopleHelper = (
+  name: string | null,
+  query: string,
+) => {
+  return name?.toLowerCase().includes(query);
+};
+
+export function getFilteredPeople(
+  filterOption: FilterType,
+  people: Person[],
+) {
+  let filteredPeople = people;
+
+  if (filterOption.sex) {
+    filteredPeople = filteredPeople
+      .filter(person => person.sex === filterOption.sex);
+  }
+
+  if (filterOption.centuries.length) {
+    filteredPeople = filteredPeople.filter(person => {
+      const personBirthCentury = Math.ceil(person.born / YEARS_IN_CENTURY);
+
+      return filterOption.centuries.includes(personBirthCentury.toString());
+    });
+  }
+
+  if (filterOption.query) {
+    filteredPeople = filteredPeople
+      .filter(person => {
+        const query = filterOption.query.toLowerCase();
+
+        return getFilteredPeopleHelper(person.name, query)
+          || getFilteredPeopleHelper(person.motherName, query)
+          || getFilteredPeopleHelper(person.fatherName, query);
+      });
+  }
+
+  return filteredPeople;
+}


These functions have nothing to do with the API, so they should be moved to a separate file

roman-mirzoian · 2023-10-09T21:41:51Z

src/components/Navbar/Navbar.tsx

+  const getLinkClass = ({ isActive }: { isActive: boolean }) => cn(
+    'navbar-item',
+    { 'has-background-grey-lighter': isActive },
+  );


It is better to define this function outside the component so that it is not created every time a new render is performed.

roman-mirzoian · 2023-10-09T21:43:35Z

src/utils/variables.ts

@@ -0,0 +1,10 @@
+export const NOT_SET_VALUE = '-';
+
+export const YEARS_IN_CENTURY = 100;


Suggested change

export const YEARS_IN_CENTURY = 100;

export const CENTURY_DIVIDER = 100;

roman-mirzoian · 2023-10-09T21:43:50Z

src/utils/variables.ts

+
+export const YEARS_IN_CENTURY = 100;
+
+export const DESC_SORT = 'desc';


Suggested change

export const DESC_SORT = 'desc';

export const DESCENDING_SORT = 'desc';

implement the ability to filter and sort people in the table

dabe14a

BudnikOleksii suggested changes Oct 6, 2023

View reviewed changes

move vars, types and enums into separate files, change logic of filte…

dbeac6d

…r function, create vars for 'magic values'

KatOlista requested a review from BudnikOleksii October 6, 2023 19:19

Moroz-Dmytro suggested changes Oct 9, 2023

View reviewed changes

fix app behavior, vars names, DRY parts, change tag a to searchLink

8545f6a

KatOlista requested a review from Moroz-Dmytro October 9, 2023 16:39

roman-mirzoian approved these changes Oct 9, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement the ability to filter and sort people in the table #516

implement the ability to filter and sort people in the table #516

KatOlista commented Oct 5, 2023

BudnikOleksii left a comment

BudnikOleksii Oct 6, 2023

BudnikOleksii Oct 6, 2023

BudnikOleksii Oct 6, 2023

BudnikOleksii Oct 6, 2023

BudnikOleksii Oct 6, 2023

BudnikOleksii Oct 6, 2023

BudnikOleksii Oct 6, 2023

BudnikOleksii Oct 6, 2023

KatOlista Oct 6, 2023

BudnikOleksii Oct 6, 2023

Moroz-Dmytro left a comment

Moroz-Dmytro Oct 9, 2023

Moroz-Dmytro Oct 9, 2023

Moroz-Dmytro Oct 9, 2023

Moroz-Dmytro Oct 9, 2023

Moroz-Dmytro Oct 9, 2023

Moroz-Dmytro Oct 9, 2023

Moroz-Dmytro Oct 9, 2023

roman-mirzoian left a comment •

edited

Loading

roman-mirzoian Oct 9, 2023

roman-mirzoian Oct 9, 2023

roman-mirzoian Oct 9, 2023

roman-mirzoian Oct 9, 2023

roman-mirzoian Oct 9, 2023

		<Route path="people" element={<PeoplePage />} />
		<Route path="people/:slug?" element={<PeoplePage />} />

-        <Route path="people" element={<PeoplePage />} />
-        <Route path="people/:slug?" element={<PeoplePage />} />
+        <Route path="people">
+          <Route path=":slug?" element={<PeoplePage />} />
+        </Route>

	export function sortPeople(people: Person[], sortParam: SortParam \| string) {
	export function getSortedPeople(people: Person[], sortParam: SortParam \| string) {

	export function filterPeople(
	export function getFilteredPeople(

		@@ -0,0 +1,10 @@
		export const NOT_SET_VALUE = '-';

		export const YEARS_IN_CENTURY = 100;

	export const YEARS_IN_CENTURY = 100;
	export const CENTURY_DIVIDER = 100;


		export const YEARS_IN_CENTURY = 100;

		export const DESC_SORT = 'desc';

	export const DESC_SORT = 'desc';
	export const DESCENDING_SORT = 'desc';

implement the ability to filter and sort people in the table #516

Are you sure you want to change the base?

implement the ability to filter and sort people in the table #516

Conversation

KatOlista commented Oct 5, 2023

BudnikOleksii left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Moroz-Dmytro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roman-mirzoian left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roman-mirzoian left a comment •

edited

Loading