Create todo app using API #767

davinnchii · 2023-09-18T07:02:58Z

polosanya

Nice start!
It is impossible to delete todos on editing

polosanya · 2023-09-21T08:16:26Z

src/App.tsx

+        <NewTodo
+          onWaiting={setTempTodo}
+        />


Suggested change

<NewTodo

onWaiting={setTempTodo}

/>

<NewTodo onWaiting={setTempTodo} />

Better write it in one line if there are < 3 props

polosanya · 2023-09-21T08:19:27Z

src/components/NewTodo/NewTodo.tsx

+  const { setErrorMessage, setIsErrorHidden } = useErrorMessage();
+  const [newTitle, setNewTitle] = useState('');
+
+  const onSubmit = (event: React.FormEvent<HTMLFormElement>) => {


Suggested change

const onSubmit = (event: React.FormEvent<HTMLFormElement>) => {

const handleSubmit = (event: React.FormEvent<HTMLFormElement>) => {

https://javascript.plainenglish.io/handy-naming-conventions-for-event-handler-functions-props-in-react-fc1cbb791364

roman-mirzoian

#767 (review) - it still doesn't work.
It would also be cool to disable the input at the time of adding a new todo:
https://gyazo.com/af36e4c7a97515b6933e89b1e45db1f5
It should also be taken into account that if the number of todos is 1, there should not be a plural in the text:

roman-mirzoian · 2023-09-21T10:28:45Z

src/App.tsx

+        .catch((error) => {
+          setErrorMessage(JSON.parse(error.message).error);
+          setIsErrorHidden(false);


You execute similar logic several times, you can move it to a separate handler:

function handleErrorMessage(error) { setErrorMessage(JSON.parse(error.message).error); setIsErrorHidden(false); } ... .catch(handleErrorMessage)

roman-mirzoian · 2023-09-21T10:32:11Z

src/components/TodoFilter/TodoFilter.tsx

+              href={key === 'All'
+                ? '#/'
+                : `#/${Filters[key][0].toLowerCase() + Filters[key].slice(1)}`}


It is better to put this logic in a separate variable for better readability

roman-mirzoian · 2023-09-21T10:32:36Z

src/components/TodoFilter/TodoFilter.tsx

+      </span>
+
+      <nav className="filter">
+        {(Object.keys(Filters) as Array<keyof typeof Filters>)


You can use Object.values right away.

DenisChakhalian

good job! let's improve your code!

video

focus is not set after double click (check the working example)
you can enable the editing of several todos at once (the editing mode is not turned off after unfocusing)
editing is not canceled after pressing Escape
but in your case there is a request to a server with the same name
input is not blocked when adding (during a request to the server), you can enter anything

BudnikOleksii

Great work 👍
Some functionality doesn't work properly, also left comments with suggestions, check them

BudnikOleksii · 2023-09-21T16:35:41Z

src/App.tsx

+    completedTodos.forEach(async ({ id }) => {
+      setLoadingTodos(prev => [...prev, { todoId: id, isLoading: true }]);
+      await deleteTodo(id);
+      try {
+        setTodos(prev => prev.filter((todo) => id !== todo.id));
+      } catch {
+        handleShowError('Unable to delete todo');
+      }
+
+      setLoadingTodos(prev => prev.filter(todo => todo.todoId !== id));
+    });
+  };


async/await doesn't work properly in forEach, use for of loop or even better to use Promise.all to handle multiple requests.

Also await deleteTodo(id); should be in try block if you want to catch errors.
Now it doesn't work as expected:
https://www.loom.com/share/78fa7ce58f064916927ee42bb3eb9fae?sid=b5c2e261-69ed-45dc-8dcb-4782a10e62fe

BudnikOleksii · 2023-09-21T16:38:34Z

src/App.tsx

+  useEffect(() => {
+    getTodos(USER_ID)
+      .then(setTodos)
+      .catch(() => {
+        handleShowError('Unable to load todos');
+      });
+  }, []);


It's a good idea to show the loader while fetching something from a server

It's a good idea to show the loader while fetching something from a server

I still don't see a loader, when user have bad internet connection they don't know is there any todos or no

BudnikOleksii · 2023-09-21T16:40:23Z

src/App.tsx

+                <div className={'modal-background'
+                  + ' has-background-white-ter'}


Suggested change

<div className={'modal-background'

+ ' has-background-white-ter'}

<div className={'modal-background has-background-white-ter'}

Why do you need to concat classes like this?

BudnikOleksii · 2023-09-21T16:41:22Z

src/App.tsx

+          <TodoFilter
+            todos={todos}
+            filterParam={filterParam}
+            onFilterChange={(newFilter) => setFilterParam(newFilter)}


Suggested change

onFilterChange={(newFilter) => setFilterParam(newFilter)}

onFilterChange={setFilterParam}

BudnikOleksii · 2023-09-21T16:44:24Z

src/components/Contexts/ErrorMessageContext.tsx

+
+export const ErrorMessageContextProvider: React.FC<Props> = ({ children }) => {
+  const [errorMessage, setErrorMessage] = useState('');
+  const [isErrorHidden, setIsErrorHidden] = useState(true);


You don't need isErrorHidden, you can check if errorMessage exists or not. An empty string is a falsy value.
https://developer.mozilla.org/en-US/docs/Glossary/Falsy
https://developer.mozilla.org/en-US/docs/Glossary/Truthy

I want my error to disappear smoothly, if I just setErrorMessage('') it will remove text and after hides the block. Which seems incorrect for me.
https://drive.google.com/file/d/1murkaSWOv1Q86OYh3rrXKQ-SDNyChk88/view?usp=sharing

BudnikOleksii · 2023-09-21T17:02:55Z

src/components/Todo/TodoTask.tsx

+        'is-active': loadingTodos.find(({ todoId }) => (
+          todoId === todo.id))?.isLoading,


You can store only ids instead of objects with todoId and isLoading, and use method some instead.
Also, move it to a vairable

BudnikOleksii · 2023-09-21T17:03:37Z

src/components/TodoFilter/TodoFilter.tsx

+  onFilterChange,
+  clearCompleted,
+}) => {
+  const itemsLeft = `${countTodos(todos, false).length} item${countTodos(todos, false).length === 1 ? '' : 's'} left`;


countTodos(todos, false) twice, create a variable

BudnikOleksii · 2023-09-21T17:04:57Z

src/components/TodoFilter/TodoFilter.tsx

+          .map((value) => {
+            const hrefValue = value === 'all'
+              ? '#/'
+              : `#/${value[0].toLowerCase() + value.slice(1)}`;


Just toLowerCase for a whole string

BudnikOleksii · 2023-09-21T17:06:06Z

src/components/TodoList/TodoList.tsx

+}) => {
+  return (
+    <>
+      {todos?.map(todo => (


Suggested change

{todos?.map(todo => (

{todos.map(todo => (

todos is required, you don't need to check

BudnikOleksii · 2023-09-21T17:07:29Z

src/types/UpdateCompleted.ts

+export interface UpdateCompleted {
+  completed: boolean,
+}


Don't create for every type a file, if it's related to Todo put it in the same file

Moroz-Dmytro

Would be good to see some error message why I can't enter empty value and save it
Also, now I can create todo with just spaces entered

The same with renaming, I can rename todo to empty(when I enter just spaces)

Moroz-Dmytro · 2023-09-22T08:27:47Z

src/App.tsx

+      case Filters.All:
+        return todos;
+
+      default:
+        return todos;


Suggested change

case Filters.All:

return todos;

default:

return todos;

case Filters.All:

default:

return todos;

Moroz-Dmytro · 2023-09-22T08:28:45Z

src/App.tsx

+  const { todos, setTodos } = useTodos();
+
+  const [tempTodo, setTempTodo] = useState<Todo | null>(null);
+  const { setLoadingTodos } = useLoadingTodos();
+  const {
+    errorMessage,
+    isErrorHidden,
+    setIsErrorHidden,
+    handleShowError,
+  } = useErrorMessage();
+
+  const [filterParam, setFilterParam] = useState(Filters.All);
+
+  const clearCompletedTodos = async () => {
+    const completedTodos = countTodos(todos, true).map(({ id }) => (


Better to keep all states together, all your own hooks also together, all constants together and all useEffects also together

Moroz-Dmytro · 2023-09-22T08:31:26Z

src/App.tsx

+        <section className="todoapp__main">
+          <TodoList todos={visibleTodos} />
+
+          {tempTodo && (


Can this tempTodo be moved to a separate component?

Moroz-Dmytro · 2023-09-22T08:31:49Z

src/App.tsx

+          onClick={() => setIsErrorHidden(true)}
+          aria-label="Delete Button"
+        />
+


Suggested change

Moroz-Dmytro · 2023-09-22T08:33:12Z

src/components/NewTodo/NewTodo.tsx

+    onWaiting({
+      id: 0,
+      completed: false,
+      title: newTitle,
+      userId: USER_ID,
+    });


Can you create constant for this object, you use the same object below

Moroz-Dmytro · 2023-09-22T08:33:52Z

src/components/NewTodo/NewTodo.tsx

+    const changingTodos = countTodos(todos, false).length
+      ? countTodos(todos, false)


Moroz-Dmytro · 2023-09-22T08:35:15Z

src/components/Todo/TodoTask.tsx

+    setLoadingTodos(prev => prev.filter(id => (
+      id !== todo.id)));


Suggested change

setLoadingTodos(prev => prev.filter(id => (

id !== todo.id)));

setLoadingTodos(prev => (

prev.filter(id => (id !== todo.id)

));

The same in other places

Moroz-Dmytro · 2023-09-22T08:36:48Z

src/components/Todo/TodoTask.tsx

+        'is-active': loadingTodos.some((id) => (
+          id === todo.id)),


Create constant for this loadingTodos.some((id) => ( id === todo.id))

Moroz-Dmytro · 2023-09-22T08:37:40Z

src/components/TodoFilter/TodoFilter.tsx

+      <button
+        type="button"
+        className="todoapp__clear-completed"
+        disabled={countTodos(todos, true).length === 0}


Create constant for this

I don't see a point to create a constant for this, I don't reuse this array in another places in this component. Also as for me this code is easy to read.

Moroz-Dmytro

🥇

Moroz-Dmytro · 2023-09-22T11:19:59Z

src/components/Todo/TodoTask.tsx

+    setLoadingTodos(prev => prev.filter(id => id !== todo.id));
+  };
+
+  const onDelete = async (todoId: number) => {


Suggested change

const onDelete = async (todoId: number) => {

const handleDeleteTodo = async (todoId: number) => {

Moroz-Dmytro · 2023-09-22T11:24:42Z

src/App.tsx

+      <div className={classNames(
+        'notification is-danger is-light has-text-weight-normal',
+        { hidden: isErrorHidden },
+      )}
+      >
+        <button
+          type="button"
+          className="delete"
+          onClick={() => setIsErrorHidden(true)}
+          aria-label="Delete Button"
+        />
+        {errorMessage}
+        <br />
+      </div>


It also can be moved to separate component

BudnikOleksii

Looks great 👍
Left a few suggestions to improve your solution.

BudnikOleksii · 2023-09-22T11:15:15Z

src/App.tsx

+  useEffect(() => {
+    getTodos(USER_ID)
+      .then(setTodos)
+      .catch(() => {
+        handleShowError('Unable to load todos');
+      });
+  }, []);


It's a good idea to show the loader while fetching something from a server

I still don't see a loader, when user have bad internet connection they don't know is there any todos or no

BudnikOleksii · 2023-09-22T11:20:57Z

src/components/NewTodo/NewTodo.tsx

+      changingTodos.forEach(({ id }) => {
+        setLoadingTodos(prev => [...prev, id]);
+      });


You add one todoId in every iteration, you can create the ids array with map method and pass them to setLoadingTodos

BudnikOleksii · 2023-09-22T11:24:37Z

src/components/NewTodo/NewTodo.tsx

+      updatedTodos.forEach((updatedTodo) => {
+        setTodos(prev => prev.map((currentTodo) => {
+          if (currentTodo.id === updatedTodo.id) {
+            return updatedTodo;
+          }
+
+          return currentTodo;
+        }));
+        setLoadingTodos(prevLoads => (
+          prevLoads.filter(id => id !== updatedTodo.id)));
+      });


Here as well

BudnikOleksii · 2023-09-22T11:26:02Z

src/components/Todo/TodoTask.tsx

+  const handleComplete = async (todoId: number,
+    event: React.ChangeEvent<HTMLInputElement>) => {


Suggested change

const handleComplete = async (todoId: number,

event: React.ChangeEvent<HTMLInputElement>) => {

const handleComplete = async (

todoId: number,

event: React.ChangeEvent<HTMLInputElement>

) => {

Fix formatting in your code in all places

BudnikOleksii · 2023-09-22T11:27:39Z

src/utils/fetchClient.ts

+/* eslint-disable @typescript-eslint/no-explicit-any */
+const BASE_URL = 'https://mate.academy/students-api';
+
+// returns a promise resolved after a given delay


Remove comments

davinnchii added 2 commits September 18, 2023 09:02

Create todo app using API

74d64c2

Update README.md

cfb8bcb

polosanya suggested changes Sep 21, 2023

View reviewed changes

davinnchii added 2 commits September 21, 2023 11:39

fix comments, refactored requests to async await

e8c4b7c

Fix comments

455b1f5

davinnchii requested a review from polosanya September 21, 2023 09:41

roman-mirzoian suggested changes Sep 21, 2023

View reviewed changes

fix comments

7d0ba51

davinnchii requested a review from roman-mirzoian September 21, 2023 12:19

disabled input while adding a new todo

8865bc7

DenisChakhalian suggested changes Sep 21, 2023

View reviewed changes

davinnchii added 2 commits September 21, 2023 16:01

fix comments

79a37d1

Added trim to title on adding todo

02c204e

BudnikOleksii suggested changes Sep 21, 2023

View reviewed changes

Fix comments

c022282

davinnchii requested a review from BudnikOleksii September 21, 2023 20:11

Moroz-Dmytro suggested changes Sep 22, 2023

View reviewed changes

fix comments

7d880c9

davinnchii requested a review from Moroz-Dmytro September 22, 2023 10:02

Moroz-Dmytro approved these changes Sep 22, 2023

View reviewed changes

BudnikOleksii approved these changes Sep 22, 2023

View reviewed changes

	const onSubmit = (event: React.FormEvent<HTMLFormElement>) => {
	const handleSubmit = (event: React.FormEvent<HTMLFormElement>) => {

		<div className={'modal-background'
		+ ' has-background-white-ter'}

	<div className={'modal-background'
	+ ' has-background-white-ter'}
	<div className={'modal-background has-background-white-ter'}

	onFilterChange={(newFilter) => setFilterParam(newFilter)}
	onFilterChange={setFilterParam}

		'is-active': loadingTodos.find(({ todoId }) => (
		todoId === todo.id))?.isLoading,

		const changingTodos = countTodos(todos, false).length
		? countTodos(todos, false)

		setLoadingTodos(prev => prev.filter(id => (
		id !== todo.id)));

	const onDelete = async (todoId: number) => {
	const handleDeleteTodo = async (todoId: number) => {

		const handleComplete = async (todoId: number,
		event: React.ChangeEvent<HTMLInputElement>) => {

Create todo app using API #767

Are you sure you want to change the base?

Create todo app using API #767

Conversation

davinnchii commented Sep 18, 2023

polosanya 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

DenisChakhalian left a comment

Choose a reason for hiding this comment

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

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

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

Moroz-Dmytro Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Moroz-Dmytro Sep 22, 2023 •

edited

Loading