Develop #792

AndrMar1939 · 2023-09-25T19:53:25Z

DEMO LINK
This is a new PR. I've closed the previous one because it didn't contain any tests.
Sorry O.Budnik, your great review remain under the old PR

#780

…Lompleted

polinavafik

Everything works as expected! great job!

polinavafik · 2023-09-26T17:03:45Z

src/Context/TodosContext/TodosProvider.tsx

+// import USER_ID from '../../helpers/USER_ID';
+// import { getTodos } from '../../api/todos';
+// import { loadTodosAction } from '../actions/actionCreators';


remove that if you don't need it anymore

Not fixed 😢

roman-mirzoian

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

roman-mirzoian · 2023-09-26T20:24:01Z

src/Components/Footer/Footer.tsx

+
+  const activeTodosNumber = getActiveTodos(todos).length;
+  const completedTodos = getCompletedTodos(todos);
+  const isClearCompletedInvisible = completedTodos.length === 0;


Suggested change

const isClearCompletedInvisible = completedTodos.length === 0;

const isClearCompletedInvisible = !completedTodos.length;

roman-mirzoian · 2023-09-26T20:24:25Z

src/Components/Footer/Footer.tsx

+  return (
+    <footer className="todoapp__footer" data-cy="Footer">
+      <span className="todo-count" data-cy="TodosCounter">
+        {`${activeTodosNumber} items left`}


If there is one item, there should not be a plural in the text.

roman-mirzoian · 2023-09-26T20:25:56Z

src/Components/Header/Header.tsx

+  const ref = useRef<HTMLInputElement>(null);
+  const [inputValue, setInputValue] = useState('');
+
+  const isToggleVisible = todos.length > 0;


Suggested change

const isToggleVisible = todos.length > 0;

const isToggleVisible = !!todos.length;

roman-mirzoian · 2023-09-26T20:26:40Z

src/Components/Header/Header.tsx

+          dispatch(patchAction);
+        });
+
+        if (rejected.length > 0) {


Suggested change

if (rejected.length > 0) {

if (rejected.length) {

roman-mirzoian · 2023-09-26T20:26:54Z

src/Components/Header/Header.tsx

+
+  return (
+    <header className="todoapp__header">
+      {/* eslint-disable jsx-a11y/control-has-associated-label */}


Try to rewrite the code so that it is not necessary to disable the linter

roman-mirzoian · 2023-09-26T20:29:31Z

src/Components/UI/ApiError.tsx

+        onClick={() => {
+          setAddClassName(true);
+        }}


Suggested change

onClick={() => {

setAddClassName(true);

}}

onClick={() => setAddClassName(true)}

roman-mirzoian · 2023-09-26T20:29:55Z

src/Context/TodosContext/TodosProvider.tsx

+// import USER_ID from '../../helpers/USER_ID';
+// import { getTodos } from '../../api/todos';
+// import { loadTodosAction } from '../actions/actionCreators';


Not fixed 😢

roman-mirzoian · 2023-09-26T20:30:11Z

src/Context/TodosContext/TodosProvider.tsx

+  // useEffect(() => {
+  //   getTodos(USER_ID)
+  //     .then((data) => {
+  //       const action = loadTodosAction(data);
+
+  //       dispatch(action);
+  //     })
+  //     .catch(e => setApiError(e));
+  // }, []);


Do not leave unnecessary comments.

roman-mirzoian · 2023-09-26T20:33:06Z

src/Components/Footer/Footer.tsx

+            const url = value === FiltersType.ALL
+              ? ''
+              : value.toLowerCase();


You can change the FiltersType a little and then you won't need to add an extra check:

export enum FiltersType { ALL = '', ACTIVE = 'Active', COMPLETED = 'Completed', } ... const url = value.toLowerCase();

roman-mirzoian · 2023-09-26T20:34:16Z

src/helpers/getFilteredTodos.ts

+    switch (filter) {
+      case FiltersType.ALL:
+      default:
+        return todos;
+
+      case FiltersType.ACTIVE:
+        return getActiveTodos(todos);
+
+      case FiltersType.COMPLETED:
+        return getCompletedTodos(todos);
+    }


Suggested change

switch (filter) {

case FiltersType.ALL:

default:

return todos;

case FiltersType.ACTIVE:

return getActiveTodos(todos);

case FiltersType.COMPLETED:

return getCompletedTodos(todos);

}

switch (filter) {

case FiltersType.ACTIVE:

return getActiveTodos(todos);

case FiltersType.COMPLETED:

return getCompletedTodos(todos);

default:

return todos;

}

AndrMar1939 · 2023-09-27T08:51:47Z

Hello mentor! There is a component Footer with handleClearCompletedClick - row 31. The handler is ok or not?

BudnikOleksii · 2023-09-29T09:55:06Z

Hello mentor! There is a component Footer with handleClearCompletedClick - row 31. The handler is ok or not?

It's ok. You can do it in different ways. You can collect requests and then handle them with Promise.all or with Promise.allsettled but we discussed that it's okay to do it without them. Only one moment that you don't need to return deletedTodos from this handler, just send requests with forEach or for of

BudnikOleksii

Nicely done 🚀

BudnikOleksii · 2023-09-29T10:01:54Z

src/Components/Header/Header.tsx

+    /* eslint-disable  @typescript-eslint/no-explicit-any */
+    Promise.allSettled(toggledTodos)
+      .then((data: any) => {


🔥
If you are interested you can google how to handle type for Promise.allSettled
https://stackoverflow.com/questions/64928212/how-to-use-promise-allsettled-with-typescript

AndrMar1939 added 5 commits September 25, 2023 18:41

new tests, work with O.Budnik review

a375224

add Promise.allSettled to Header, rewrite deleteAll in Footer;

c272cff

add Promise.allSettled to Header, rewrite deleteAll in Footer;

93e1e44

create separate Providers, made smth strange in Footer in handleClear…

c019a0a

…Lompleted

add Transitiongroup

7e67faa

polinavafik approved these changes Sep 26, 2023

View reviewed changes

roman-mirzoian suggested changes Sep 26, 2023

View reviewed changes

fix syntax in conditions, rewrite Filter type

6c7c334

BudnikOleksii approved these changes Sep 29, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Develop #792

Develop #792

AndrMar1939 commented Sep 25, 2023 •

edited

Loading

polinavafik left a comment

polinavafik Sep 26, 2023

roman-mirzoian Sep 26, 2023

roman-mirzoian left a comment

roman-mirzoian Sep 26, 2023

roman-mirzoian Sep 26, 2023

roman-mirzoian Sep 26, 2023

roman-mirzoian Sep 26, 2023

roman-mirzoian Sep 26, 2023

roman-mirzoian Sep 26, 2023

roman-mirzoian Sep 26, 2023

roman-mirzoian Sep 26, 2023

roman-mirzoian Sep 26, 2023

AndrMar1939 Sep 27, 2023

roman-mirzoian Sep 26, 2023

AndrMar1939 commented Sep 27, 2023 •

edited

Loading

BudnikOleksii commented Sep 29, 2023

BudnikOleksii left a comment

BudnikOleksii Sep 29, 2023

	const isClearCompletedInvisible = completedTodos.length === 0;
	const isClearCompletedInvisible = !completedTodos.length;

	const isToggleVisible = todos.length > 0;
	const isToggleVisible = !!todos.length;

Develop #792

Are you sure you want to change the base?

Develop #792

Conversation

AndrMar1939 commented Sep 25, 2023 • edited Loading

polinavafik left a 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

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

AndrMar1939 commented Sep 27, 2023 • edited Loading

BudnikOleksii commented Sep 29, 2023

BudnikOleksii left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrMar1939 commented Sep 25, 2023 •

edited

Loading

AndrMar1939 commented Sep 27, 2023 •

edited

Loading