Develop #519

cty2802 · 2023-10-05T17:03:43Z

polosanya

Nice! Let's improve your solution a bit)
The filters shouldn't reset when we select a person

polosanya · 2023-10-09T07:49:46Z

src/components/PeopleSort/PeopleSort.tsx

+    if (sort) {
+      params.set('sort', sort);
+    } else {
+      params.delete('sort');
+    }
+
+    if (sort === oldSort && !oldOrder) {
+      params.set('order', 'desc');
+    } else {
+      params.delete('order');


Consider using switch...case here instead

I considered using switch...case here, but I feel the code looks too complicated with it. The thing that I should check not only the 'sort' variable, but the 'oldOrder' variable too. Still, I would appreciate it if you give me an example of how it can be done nicely.

@cty2802 I would shorten the code like this:

if (sort === oldSort) { params.delete('sort'); params.delete('order'); } else { params.set('sort', sort); params.delete('order'); if (oldOrder) { params.set('order', 'desc'); } }

but it is worth analyzing in practice whether everything will work as it should 😄

Unfortunately, it doesn't work. Under such conditions, sorting has only two states - asc and start position, but there is no desc position.
I like my code, can I keep it? And we will assume that it's not a bug, it's a feature -:)

@cty2802 of course, I only made assumptions based on your code, but if my approach does not work - then we leave it as you did))
One of the main rules of programming - if it works, don't touch it 😅

polosanya · 2023-10-09T07:50:12Z

src/helper.tsx

+  const humans: Person[] = [...people];
+
+  return humans.sort((humanA: Person, humanB: Person): number => {
+    if (oldSort === SortBy.Name) {
+      return oldOrder === SortOrder.Desc
+        ? (humanA.name.localeCompare(humanB.name) * -1)
+        : humanA.name.localeCompare(humanB.name);
+    }
+
+    if (oldSort === SortBy.Sex) {


polosanya · 2023-10-09T07:50:43Z

src/pages/PeoplePage.tsx

+export const PeoplePage: React.FC = () => {
+  const [people, setPeople] = useState<Person[]>([]);
+  const [isLoading, setIsLoading] = useState(true);
+  const [errorMassage, setErrorMassage] = useState('');


Suggested change

const [errorMassage, setErrorMassage] = useState('');

const [errorMessage, setErrorMessage] = useState('');

roman-mirzoian

Good job, but there is quite a bit of work to be done.
Also, if there are no search results, we have to see only the message, without the table:

roman-mirzoian · 2023-10-11T14:27:37Z

src/components/PeopleSort/PeopleSort.tsx

+  const setIconClass = (columnName: SortBy): string => {
+    if (columnName === oldSort && !oldOrder) {
+      return 'fas fa-sort-up';
+    }
+
+    if (columnName === oldSort && oldOrder) {
+      return 'fas fa-sort-down';
+    }
+
+    return 'fas fa-sort';
+  };


Use the classnames library for this logic

roman-mirzoian · 2023-10-11T14:27:48Z

src/components/PeopleSort/PeopleSort.tsx

+            }}
+          >
+            <span className="icon">
+              <i className={setIconClass(SortBy.Name)} />


classnames issue

roman-mirzoian · 2023-10-11T14:36:42Z

src/components/PeopleSort/PeopleSort.tsx

+    if (sort) {
+      params.set('sort', sort);
+    } else {
+      params.delete('sort');
+    }
+
+    if (sort === oldSort && !oldOrder) {
+      params.set('order', 'desc');
+    } else {
+      params.delete('order');


@cty2802 I would shorten the code like this:

if (sort === oldSort) { params.delete('sort'); params.delete('order'); } else { params.set('sort', sort); params.delete('order'); if (oldOrder) { params.set('order', 'desc'); } }

but it is worth analyzing in practice whether everything will work as it should 😄

roman-mirzoian · 2023-10-11T14:41:39Z

src/pages/PeoplePage.tsx

+  return !centuries?.length
+    ? true
+    : centuries.includes(Math.floor(person.born / 100).toString());


Suggested change

return !centuries?.length

? true

: centuries.includes(Math.floor(person.born / 100).toString());

return !centuries?.length || centuries.includes(Math.floor(person.born / 100).toString());

But avoid usign "magic values" (100)

…ge, without the table"

roman-mirzoian

Well done 👍

roman-mirzoian · 2023-10-12T10:13:33Z

src/components/PeopleSort/PeopleSort.tsx

+  const setIconClass = (columnName: SortBy): string => {
+    return classNames('fas',
+      { 'fa-sort-up': columnName === oldSort && !oldOrder },
+      { 'fa-sort-down': columnName === oldSort && oldOrder },
+      { 'fa-sort': columnName });
+  };


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-12T10:13:41Z

src/components/PeopleSort/PeopleSort.tsx

+  const oldSort = searchParams.get('sort');
+  const oldOrder = searchParams.get('order');
+
+  const setIconClass = (columnName: SortBy): string => {


Suggested change

const setIconClass = (columnName: SortBy): string => {

const getIconClass = (columnName: SortBy): string => {

roman-mirzoian · 2023-10-12T10:15:01Z

src/components/PeopleSort/PeopleSort.tsx

+    if (sort) {
+      params.set('sort', sort);
+    } else {
+      params.delete('sort');
+    }
+
+    if (sort === oldSort && !oldOrder) {
+      params.set('order', 'desc');
+    } else {
+      params.delete('order');


@cty2802 of course, I only made assumptions based on your code, but if my approach does not work - then we leave it as you did))
One of the main rules of programming - if it works, don't touch it 😅

…outside the component

cty2802 added 3 commits October 3, 2023 21:24

fixed syntax (BodyTable)

328a017

add block Filter

04c760c

add task solution

906dbbc

polosanya suggested changes Oct 9, 2023

View reviewed changes

cty2802 added 3 commits October 11, 2023 13:35

renamed errorMassage --> errorMessage

d3dbf3f

used switch for oldSort (helper.js)

bbc8a88

some stripping

cad0281

cty2802 requested a review from polosanya October 11, 2023 13:37

roman-mirzoian suggested changes Oct 11, 2023

View reviewed changes

cty2802 added 4 commits October 11, 2023 18:28

reduced design if (PeopleSort.js)

743cb9f

reduced function filterByCentury (PeoplePage.tsx), removed "magic value"

d4bd85f

used classnames library (PeopleSort.tsx)

c5d143f

fixed " if there are no search results, we have to see only the messa…

32e3d50

…ge, without the table"

cty2802 requested a review from roman-mirzoian October 11, 2023 16:43

roman-mirzoian approved these changes Oct 12, 2023

View reviewed changes

renamed setIconClass --> setIconClass, defined function setIconClass …

5f85b5f

…outside the component

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Develop #519

Develop #519

cty2802 commented Oct 5, 2023

polosanya left a comment

polosanya Oct 9, 2023

cty2802 Oct 11, 2023 •

edited

Loading

roman-mirzoian Oct 11, 2023

cty2802 Oct 11, 2023 •

edited

Loading

roman-mirzoian Oct 12, 2023

polosanya Oct 9, 2023

polosanya Oct 9, 2023

roman-mirzoian left a comment

roman-mirzoian Oct 11, 2023

roman-mirzoian Oct 11, 2023

roman-mirzoian Oct 11, 2023

roman-mirzoian Oct 11, 2023

roman-mirzoian Oct 11, 2023

roman-mirzoian left a comment

roman-mirzoian Oct 12, 2023

roman-mirzoian Oct 12, 2023

roman-mirzoian Oct 12, 2023

	const [errorMassage, setErrorMassage] = useState('');
	const [errorMessage, setErrorMessage] = useState('');

	const setIconClass = (columnName: SortBy): string => {
	const getIconClass = (columnName: SortBy): string => {

Develop #519

Are you sure you want to change the base?

Develop #519

Conversation

cty2802 commented Oct 5, 2023

polosanya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cty2802 Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cty2802 Oct 11, 2023 • 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

roman-mirzoian 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

roman-mirzoian 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

cty2802 Oct 11, 2023 •

edited

Loading

cty2802 Oct 11, 2023 •

edited

Loading