Implement_a_TODO_app #717

rialleons · 2023-10-05T15:08:13Z

DEMO LINK

loralevitska

Good job! There are things that should be fixed:

add right condition for 1 item left
remove outline, it breaks design

loralevitska · 2023-10-05T15:15:01Z

src/components/Footer/Footer.scss

@@ -0,0 +1,53 @@
+.footer {
+  color: #777;


It is a good practice to make all colors variable

loralevitska · 2023-10-05T15:15:35Z

src/components/Header/Header.tsx

+    }
+
+    if (event.key === 'Enter') {
+      if (newTodoTitle.trim().length !== 0) {


Suggested change

if (newTodoTitle.trim().length !== 0) {

if (newTodoTitle.trim().length) {

loralevitska · 2023-10-05T15:17:33Z

src/components/Main/Main.tsx

+  const dispatch = useContext(DispatchContext);
+  const filterStatus = useContext(FilterContext);
+
+  const [checked, setChecked] = useState(false);


Rename boolean variable according tips. Fix all cases if present

Suggested change

const [checked, setChecked] = useState(false);

const [isChecked, setIsChecked] = useState(false);

loralevitska · 2023-10-05T15:18:02Z

src/components/TodoItem/TodoItem.tsx

+import React, {
+  useContext, useRef, useState, useEffect,
+} from 'react';


Suggested change

import React, {

useContext, useRef, useState, useEffect,

} from 'react';

import React, {

useContext,

useRef,

useState,

useEffect,

} from 'react';

loralevitska · 2023-10-05T15:18:53Z

src/utils/manageLocalState.ts

+export const getStartingState = (): Todo[] => {
+  const data = localStorage.getItem(key);
+
+  if (data === null) {


Suggested change

if (data === null) {

if (!data) {

etojeDenys · 2023-10-06T07:15:31Z

src/components/Footer/Footer.tsx

+        ) : (
+          `${notCompletedLength} items left`
+        )}
+        {}


looks redundant

etojeDenys · 2023-10-06T07:16:31Z

src/components/Main/Main.tsx

+      break;
+
+    case Status.Active:
+      filteredTodos = [...todos].filter(todo => !todo.completed);


Suggested change

filteredTodos = [...todos].filter(todo => !todo.completed);

filteredTodos = todos.filter(todo => !todo.completed);

filter doesn't change the array, so this copy is redundant

volodymyr-soltys97

Good job 👍
Let's improve your code

volodymyr-soltys97 · 2023-10-06T08:34:57Z

src/components/Footer/Footer.scss

+  @media (max-width: 430px) {
+    & {
+      height: 50px;
+    }
+  }


Suggested change

@media (max-width: 430px) {

& {

height: 50px;

}

}

@media (max-width: 430px) {

height: 50px;

}

volodymyr-soltys97 · 2023-10-06T08:35:10Z

src/components/Footer/Footer.scss

+  & strong {
+    font-weight: 300;
+  }


Suggested change

& strong {

font-weight: 300;

}

strong {

font-weight: 300;

}

volodymyr-soltys97 · 2023-10-06T08:36:11Z

src/components/Footer/Footer.tsx

+        {notCompletedLength === 1 ? (
+          '1 item left'
+        ) : (
+          `${notCompletedLength} items left`
+        )}


You can move it to the helper function and use it here

volodymyr-soltys97 · 2023-10-06T08:36:55Z

src/components/Main/Main.scss

+    & {
+      background: none;
+    }


The same as above, need to fix it everywhere

Suggested change

& {

background: none;

}

background: none;

volodymyr-soltys97 · 2023-10-06T08:39:08Z

src/components/TodosContext/TodosContext.tsx

+    <SetFilterContext.Provider value={setCurrentFilter}>
+      <FilterContext.Provider value={currentFilter}>
+        <DispatchContext.Provider value={dispatch}>
+          <StateContext.Provider value={todos}>
+            {children}
+          </StateContext.Provider>
+        </DispatchContext.Provider>
+      </FilterContext.Provider>
+    </SetFilterContext.Provider>


Do you need so many providers?
You can create only 1 provider and pass an object with data to its value

IvanFesenko

Do not forget check my comments

IvanFesenko · 2023-10-06T12:35:16Z

src/components/Main/Main.tsx

+  let filteredTodos: Todo[] = [];
+
+  switch (currentFilter) {
+    case Status.All:
+      filteredTodos = [...todos];
+      break;
+
+    case Status.Active:
+      filteredTodos = todos.filter(todo => !todo.completed);
+      break;
+
+    case Status.Completed:
+      filteredTodos = todos.filter(todo => todo.completed);
+      break;
+
+    default:
+      throw new Error('Unknown filter!');
+  }


My assumption is move this logic into helper function to get visible todos it will make your component cleaner
const visibleTodos = getVisibleTodos(todos, currentFilter);

IvanFesenko · 2023-10-06T12:39:23Z

src/components/TodosFilter/TodosFilter.tsx

+  const { currentFilter, setCurrentFilter } = useContext(TodosContext);
+
+  return (
+    <ul className="filters" data-cy="todosFilter">
+      <li>
+        <a
+          href="#/"
+          className={classNames({
+            selected: currentFilter === Status.All,
+          })}
+          onClick={() => setCurrentFilter(Status.All)}
+        >
+          {Status.All}
+        </a>
+      </li>
+
+      <li>
+        <a
+          href="#/active"
+          className={classNames({
+            selected: currentFilter === Status.Active,
+          })}
+          onClick={() => setCurrentFilter(Status.Active)}
+        >
+          {Status.Active}
+        </a>
+      </li>
+
+      <li>
+        <a
+          href="#/completed"
+          className={classNames({
+            selected: currentFilter === Status.Completed,
+          })}
+          onClick={() => setCurrentFilter(Status.Completed)}
+        >
+          {Status.Completed}
+        </a>


Also recomendation do not use anonymous function in JSX

Suggested change

const { currentFilter, setCurrentFilter } = useContext(TodosContext);

return (

<ul className="filters" data-cy="todosFilter">

<li>

<a

href="#/"

className={classNames({

selected: currentFilter === Status.All,

})}

onClick={() => setCurrentFilter(Status.All)}

>

{Status.All}

</a>

</li>

<li>

<a

href="#/active"

className={classNames({

selected: currentFilter === Status.Active,

})}

onClick={() => setCurrentFilter(Status.Active)}

>

{Status.Active}

</a>

</li>

<li>

<a

href="#/completed"

className={classNames({

selected: currentFilter === Status.Completed,

})}

onClick={() => setCurrentFilter(Status.Completed)}

>

{Status.Completed}

</a>

const { currentFilter, setCurrentFilter } = useContext(TodosContext);

const onFilterSelect = (status: Status) => () => {

setCurrentFilter(status);

}

return (

<ul className="filters" data-cy="todosFilter">

<li>

<a

href="#/"

className={classNames({

selected: currentFilter === Status.All,

})}

onClick={onFilterSelect(Status.All)}

>

{Status.All}

</a>

</li>

<li>

<a

href="#/active"

className={classNames({

selected: currentFilter === Status.Active,

})}

onClick={onFilterSelect(Status.Active)}

>

{Status.Active}

</a>

</li>

<li>

<a

href="#/completed"

className={classNames({

selected: currentFilter === Status.Completed,

})}

onClick={onFilterSelect(Status.Completed)}

>

{Status.Completed}

</a>

rialleons added 4 commits October 4, 2023 14:47

Change username in readme

3c9cec1

Save to local storage

8b20056

Implement a TODO app

c055fa8

Separate styles and moved utils

7fb9402

loralevitska suggested changes Oct 5, 2023

View reviewed changes

rialleons added 2 commits October 5, 2023 21:13

Make all colors variables, remove outline, make small code fixes

666377e

Correct switch case for filtering

406bdf9

rialleons requested a review from loralevitska October 6, 2023 04:00

etojeDenys suggested changes Oct 6, 2023

View reviewed changes

Remove redundant array copies

31f429d

rialleons requested a review from etojeDenys October 6, 2023 08:25

volodymyr-soltys97 suggested changes Oct 6, 2023

View reviewed changes

Remove unnecessary ampersands in sccs and accumulate react contexts

afe1cef

rialleons requested a review from volodymyr-soltys97 October 6, 2023 11:57

IvanFesenko approved these changes Oct 6, 2023

View reviewed changes

Add function for visible todos and replace anonymous functions

eec9c32

rialleons force-pushed the develop branch from 46dd5fc to eec9c32 Compare October 6, 2023 13:04

Hide overflow while editing on mobile

3708904

rialleons force-pushed the develop branch from e311c7f to 3708904 Compare October 8, 2023 08:54

rialleons added 2 commits October 9, 2023 22:33

Add package-lock.json

c4b7be0

Give proper attribute to input fields and fix redacting

9502287

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement_a_TODO_app #717

Implement_a_TODO_app #717

rialleons commented Oct 5, 2023

loralevitska left a comment

loralevitska Oct 5, 2023

loralevitska Oct 5, 2023

loralevitska Oct 5, 2023

loralevitska Oct 5, 2023

loralevitska Oct 5, 2023

etojeDenys Oct 6, 2023

etojeDenys Oct 6, 2023

volodymyr-soltys97 left a comment

volodymyr-soltys97 Oct 6, 2023

volodymyr-soltys97 Oct 6, 2023

volodymyr-soltys97 Oct 6, 2023

volodymyr-soltys97 Oct 6, 2023

volodymyr-soltys97 Oct 6, 2023

IvanFesenko left a comment

IvanFesenko Oct 6, 2023

IvanFesenko Oct 6, 2023

	if (newTodoTitle.trim().length !== 0) {
	if (newTodoTitle.trim().length) {

	const [checked, setChecked] = useState(false);
	const [isChecked, setIsChecked] = useState(false);

	filteredTodos = [...todos].filter(todo => !todo.completed);
	filteredTodos = todos.filter(todo => !todo.completed);

Implement_a_TODO_app #717

Are you sure you want to change the base?

Implement_a_TODO_app #717

Conversation

rialleons commented Oct 5, 2023

loralevitska 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

volodymyr-soltys97 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

IvanFesenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment