Solution #1051

YaroslavKolesnyk · 2024-09-08T14:13:41Z

OleksiiKislukhin

Looks good! Just need few small change.

Changing the filter by sex should not reset other filters. The state of all other filters should be preserved while applying the new filter for sex.
Keep search params when navigating within the People page (when selecting a person or clicking the People link).
The query match should occur not only on the name but also on the motherName and fatherName.

igorkruz

GJ 🔥
let's make some improvements

1 - All the filters and sort params should be saved as URL Search Params, so you could share the link to show exactly what you see.
2 - Keep search params when navigating within the People page (when selecting a person or clicking the People link).

igorkruz · 2024-09-09T11:17:08Z

src/Root.tsx

+      <Routes>
+        <Route path="/" element={<App />}>
+          <Route path="*" element={<PageNotFound />} />
+
+          <Route index element={<HomePageTitle />} />
+
+          <Route path="people">
+            <Route path=":slugParam?" element={<PeoplePage />} />
+          </Route>
+
+          <Route path="home" element={<Navigate to="/" replace />} />
+        </Route>


let's create Routes enum

igorkruz · 2024-09-09T11:19:18Z

src/components/PeoplePage.tsx

+  const activeCenturies = searchParams.getAll('centuries') || '';
+  const activeSex = searchParams.get('sex') || '';
+  const activeQuery = searchParams.get('query') || '';


let's replace strings with enum

igorkruz · 2024-09-09T11:19:44Z

src/components/PeoplePage.tsx

+      case 'm':
+        return peopleList.filter(person => person.sex === activeSex);
+      case 'f':
+        return peopleList.filter(person => person.sex === activeSex);


enum for sex

igorkruz · 2024-09-09T11:21:54Z

src/components/PeoplePage.tsx

+    const fetchPeople = async () => {
+      setFetchPeopleError(false);
+
+      try {
+        const response = await getPeople();
+
+        setPeople(response);
+        setShowFilters(true);
+      } catch (error) {
+        setFetchPeopleError(true);
+      } finally {
+        setLoading(false);
+      }
+    };


move this function out of the useEffect hook

VolodymyrKirichenko

Overall - good job 👍
But review my comments and fix them)

VolodymyrKirichenko · 2024-09-09T14:51:57Z

src/Root.tsx

+} from 'react-router-dom';
+
+import { App } from './App';
+// eslint-disable-next-line import/extensions


do u need this disable?why?

VolodymyrKirichenko · 2024-09-09T14:56:14Z

src/components/PeopleFilters.tsx

+  activeCenturies,
+}: PeopleFilterProps) => {
+  const [activeFilter, setActiveFilter] = useState('');
+  const centuryList = [16, 17, 18, 19, 20];


move it to utils directory. consts file

VolodymyrKirichenko · 2024-09-09T14:57:49Z

src/components/PeopleFilters.tsx

+          onClick={e => {
+            e.preventDefault();
+            const newSearchParams = new URLSearchParams(searchParams);
+            newSearchParams.delete('sex');
+            setSearchParams(newSearchParams);
+          }}


use separate functions whenever u need a handle function

VolodymyrKirichenko · 2024-09-09T14:58:50Z

src/components/PeopleFilters.tsx

+          onClick={e => {
+            e.preventDefault();
+            const newSearchParams = new URLSearchParams(searchParams);
+            newSearchParams.set('sex', 'm');
+            setSearchParams(newSearchParams);
+          }}
+        >


u can make 1 universal function for all cases

VolodymyrKirichenko · 2024-09-09T14:59:20Z

src/components/PeoplePage.tsx


 export const PeoplePage = () => {
+  // #region states


remove all the comments from project

VolodymyrKirichenko · 2024-09-09T15:04:44Z

src/components/PeoplePage.tsx


 export const PeoplePage = () => {
+  // #region states
+  const [loading, setLoading] = useState(true);
+  const [fetchPeopleError, setFetchPeopleError] = useState(false);


Fetch??? This prefix is only for fetch functions, NOT for booleans. Change it!

VolodymyrKirichenko · 2024-09-09T15:07:24Z

src/components/PeoplePage.tsx

+  const fetchingErrorNotification = !loading && fetchPeopleError;
+  const noPeopleNotification = !loading && !fetchPeopleError && !people.length;
+  const showPeopleTable = !loading && !fetchPeopleError && !!people.length;


If there is an error or an active loader - ur list length will be 0.
u don't need to constantly duplicate conditions

Suggested change

const fetchingErrorNotification = !loading && fetchPeopleError;

const noPeopleNotification = !loading && !fetchPeopleError && !people.length;

const showPeopleTable = !loading && !fetchPeopleError && !!people.length;

const fetchingErrorNotification = fetchPeopleError;

const noPeopleNotification = !people.length;

const showPeopleTable = !!people.length;

VolodymyrKirichenko · 2024-09-09T15:08:27Z

src/components/Person.tsx

+
+export const Person: React.FC<Props> = ({ person, people }) => {
+  const { slugParam } = useParams();
+  const { name, sex, born, died, slug, motherName, fatherName } = person;


if u have 3 or more parameters - move them to separate line

Suggested change

const { name, sex, born, died, slug, motherName, fatherName } = person;

const {

name,

sex,

born,

died,

slug,

motherName,

fatherName

} = person;

VolodymyrKirichenko · 2024-09-09T15:09:59Z

src/components/Person.tsx

+  return (
+    <tr
+      data-cy="person"
+      className={person.slug === slugParam ? 'has-background-warning' : ''}


u always used the library - classNames. Use it here too.
Never mix ur code styles

VolodymyrKirichenko · 2024-09-09T15:12:03Z

src/page/HomePageTitle.tsx

@@ -0,0 +1 @@
+export const HomePageTitle = () => <h1 className="title">Home Page</h1>;


i see u have 3 similar components with same styles where only the header changes.
Make 1 component that will accept the title as props

igorkruz

Good Job 🔥
let's make some improvements

igorkruz · 2024-09-10T09:24:22Z

src/components/PeopleFilters.tsx

+    setActiveFilter(searchParams.get('sex') || '');
+    setQuery(searchParams.get('query') || '');


you have the enum QueryParams so use it

Suggested change

setActiveFilter(searchParams.get('sex') || '');

setQuery(searchParams.get('query') || '');

setActiveFilter(searchParams.get(QueryParams.Sex) || '');

setQuery(searchParams.get(QueryParams.Query) || '');

igorkruz · 2024-09-10T09:25:26Z

src/components/PeopleFilters.tsx

+    const newSearchParams = new URLSearchParams(searchParams);
+
+    if (!event.target.value) {
+      newSearchParams.delete('query');


use enum QueryParams

Suggested change

newSearchParams.delete('query');

newSearchParams.delete(QueryParams.Query);

igorkruz · 2024-09-10T09:26:08Z

src/components/PeopleFilters.tsx

+    if (!event.target.value) {
+      newSearchParams.delete('query');
+    } else {
+      newSearchParams.set('query', event.target.value);


the same here, use enum QueryParams

igorkruz · 2024-09-10T09:29:33Z

src/components/PeopleFilters.tsx

+  const handleResetSexFilter = (e: React.MouseEvent<HTMLAnchorElement>) => {
+    e.preventDefault();
+    const newSearchParams = new URLSearchParams(searchParams);
+    newSearchParams.delete('sex');


use enum QueryParams

igorkruz · 2024-09-10T09:31:50Z

src/components/PeopleFilters.tsx

+  const handleSetMaleFilter = (e: React.MouseEvent<HTMLAnchorElement>) => {
+    e.preventDefault();
+    const newSearchParams = new URLSearchParams(searchParams);
+    newSearchParams.set('sex', 'm');


you have created the enum QueryParams and enum Sex use them in cases like this

igorkruz · 2024-09-10T09:35:52Z

src/components/PeopleFilters.tsx

+  const handleSetFemaleFilter = (e: React.MouseEvent<HTMLAnchorElement>) => {
+    e.preventDefault();
+    const newSearchParams = new URLSearchParams(searchParams);
+    newSearchParams.set('sex', 'f');


the same as above, use enum QueryParams and enum Sex

VolodymyrKirichenko

Good job 🔥

OleksiiKislukhin suggested changes Sep 9, 2024

View reviewed changes

solution1.1

1ddf996

YaroslavKolesnyk force-pushed the develop branch from 9cae7b1 to 1ddf996 Compare September 9, 2024 10:50

YaroslavKolesnyk requested a review from OleksiiKislukhin September 9, 2024 10:59

igorkruz suggested changes Sep 9, 2024

View reviewed changes

add task solution

40fbcc0

YaroslavKolesnyk requested a review from igorkruz September 9, 2024 13:08

VolodymyrKirichenko suggested changes Sep 9, 2024

View reviewed changes

add task solution

d85a759

YaroslavKolesnyk requested a review from VolodymyrKirichenko September 9, 2024 16:12

igorkruz suggested changes Sep 10, 2024

View reviewed changes

solution2.1

b03359a

YaroslavKolesnyk requested a review from igorkruz September 10, 2024 09:59

VolodymyrKirichenko approved these changes Sep 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Solution #1051

Solution #1051

YaroslavKolesnyk commented Sep 8, 2024

OleksiiKislukhin left a comment

igorkruz left a comment

igorkruz Sep 9, 2024

igorkruz Sep 9, 2024

igorkruz Sep 9, 2024

igorkruz Sep 9, 2024

VolodymyrKirichenko left a comment

VolodymyrKirichenko Sep 9, 2024

VolodymyrKirichenko Sep 9, 2024

VolodymyrKirichenko Sep 9, 2024

VolodymyrKirichenko Sep 9, 2024

VolodymyrKirichenko Sep 9, 2024

VolodymyrKirichenko Sep 9, 2024

VolodymyrKirichenko Sep 9, 2024

VolodymyrKirichenko Sep 9, 2024

VolodymyrKirichenko Sep 9, 2024

VolodymyrKirichenko Sep 9, 2024

igorkruz left a comment

igorkruz Sep 10, 2024

igorkruz Sep 10, 2024

igorkruz Sep 10, 2024

igorkruz Sep 10, 2024

igorkruz Sep 10, 2024

igorkruz Sep 10, 2024

VolodymyrKirichenko left a comment

		@@ -0,0 +1 @@
		export const HomePageTitle = () => <h1 className="title">Home Page</h1>;

		setActiveFilter(searchParams.get('sex') \|\| '');
		setQuery(searchParams.get('query') \|\| '');

	newSearchParams.delete('query');
	newSearchParams.delete(QueryParams.Query);

Solution #1051

Are you sure you want to change the base?

Solution #1051

Conversation

YaroslavKolesnyk commented Sep 8, 2024

OleksiiKislukhin left a comment

Choose a reason for hiding this comment

igorkruz 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

VolodymyrKirichenko 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

igorkruz 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

VolodymyrKirichenko left a comment

Choose a reason for hiding this comment