Everything works outside of escape. Would love some advice on that. #772

szymonpachucki · 2023-09-20T11:23:38Z

domalewski

It's working so I guess it's ok
Removed the unnecessary comments

domalewski · 2023-09-24T10:07:20Z

src/App.tsx

-    return <UserWarning />;
-  }
+  const [todos, setTodos] = useState<Todo[]>([
+    // Initialize with some example todos


please remove comment, code it's simple to understand

domalewski · 2023-09-24T10:12:33Z

src/components/footer.tsx

+}
+
+// eslint-disable-next-line max-len
+export const Footer: React.FC<Props> = ({ handleSetFilter, todos, setTodos }) => {


Suggested change

export const Footer: React.FC<Props> = ({ handleSetFilter, todos, setTodos }) => {

export const Footer: React.FC<Props> = ({

handleSetFilter,

todos,

setTodos }) => {

I know it's looks worst but then you don't have to disabling eslint

domalewski

It's working so I guess it's ok
Removed the unnecessary comments

choeqq

please add comments to your code only when you want to implement some normally unexpected behaviour

consider using context in your app

choeqq · 2023-09-27T07:09:10Z

src/components/footer.tsx

current filter should have different styles, when picked, as a user I want to know what kind of todos the app is displaying

choeqq · 2023-09-27T07:10:09Z

src/App.tsx

+  useEffect(() => {
+    // You can perform any additional actions here when the todos state changes.
+  }, [todos]);


empty effect?

choeqq · 2023-09-27T07:10:32Z

src/App.tsx

+
+      {/* Notification is shown in case of any error */}
+      {/* Add the 'hidden' class to hide the message smoothly */}
+      {error !== null && (


Suggested change

{error !== null && (

{error && (

choeqq · 2023-09-27T07:12:20Z

src/components/inputOfTodos.tsx

+
+  return (
+    <header className="todoapp__header">
+      {todos.length !== 0 && (


Suggested change

{todos.length !== 0 && (

{todos.length && (

choeqq · 2023-09-27T07:13:44Z

src/components/task.tsx

+        <input
+          ref={inputRef}
+          type="text"
+          className="todoapp__new"
+          placeholder="Empty todo will be deleted"
+          value={inputValue}
+          onChange={handleInputChange}
+          onKeyPress={handleInputKeyPress}
+          onBlur={handleInputBlur}
+        />
+      )}


make sure to apply correct styles to editing input - it should look the same as Todo styles

choeqq · 2023-09-27T07:14:12Z

src/components/todoList.tsx

+
+  return (
+    <section className="todoapp__main">
+      {visibleTodos().map((todo) => (


Suggested change

{visibleTodos().map((todo) => (

{visibleTodos.map((todo) => (

this should be a value instead of a function call

mbruhler

For the problem with cancel edit when Esc key is up, you can do this in two ways:

register a listener for this button in useEffect and remove it on component unmount. If escape is pressed then trigger a function that changes the edit state
register an event listener on the input using onKeyUp and handling it there, similar to what you did with Enter.

I think 2. is better, because the scope of the event handler is just for the input, but I wanted to show you that there is also a possibility to register such event globally for component, not just input :)

mbruhler · 2023-09-27T09:59:14Z

src/App.tsx

-  }
+  const [todos, setTodos] = useState<Todo[]>([]);
+  const [error, setError] = useState<Errors | null>(null);
+  const [filterTodos, setFilterTodos] = useState<FilterOption>('All');


It's perfect that you created a type for the filter, but it shouldn't be a string value, rather an enum or JSON with only possible values.

To do so:
Change FilterOptions type to const enum and provide the possible values:

export const enum FilterOption { ALL = 'all', ACTIVE = 'active', COMPLETED = 'completed', }

Then you can use it as a type and as a value to compare
const [filterTodos, setFilterTodos] = useState<FilterOption>(FilterOption.ALL);

You can also use a JSON value here rather than const enum and type it accordingly, but I think the const enum is simpler and is nice to know about them, especially in such cases.

mbruhler · 2023-09-27T10:00:10Z

src/types/filterOption.ts

@@ -0,0 +1 @@
+export type FilterOption = 'All' | 'Active' | 'Completed';


Change type according to const enum change request.

mbruhler · 2023-09-27T10:01:25Z

src/components/todoList.tsx

+  const visibleTodos = (() => {
+    let filteredTodos = todos;
+
+    if (filterTodos === 'Active') {


With the const enum change, you should do:

if (filterTodos === FilterOption.ACTIVE) { ... }

Change others too.

mbruhler · 2023-09-27T10:03:16Z

src/components/inputOfTodos.tsx

+
+  return (
+    <header className="todoapp__header">
+      {/* i can not write it as {todos.lenght && as when its value is 0 it will display  '0' on the screen  */}


yeah, the condition you've provided is fine, please clear the comment

pawel-peksa

Good job! 💪 Almost done!
-> Added one minor comment regarding the performance optimization. (Try to avoid unnecessary itarations in your code)

pawel-peksa · 2023-09-29T11:11:43Z

src/components/inputOfTodos.tsx

+      {todos.length > 0 && (
+        <button
+          type="button"
+          className={`todoapp__toggle-all ${todos.every((t) => t.completed) ? 'active' : ''}`}


You itarate over the array of Todos in two places to get the same information (see line 38). This is not the best performance optimization. Let's try to do this only once and use the result in both places.

choeqq

LGTM ✅

szymonpachucki added 2 commits September 20, 2023 13:22

Everything works outside of escape. Would love some advice on that.

7a54662

fixed counter

ae96632

domalewski reviewed Sep 24, 2023

View reviewed changes

domalewski approved these changes Sep 24, 2023

View reviewed changes

choeqq suggested changes Sep 27, 2023

View reviewed changes

add task solution

060a99f

szymonpachucki requested a review from choeqq September 27, 2023 09:06

mbruhler suggested changes Sep 27, 2023

View reviewed changes

add task solution

1437e35

szymonpachucki requested a review from mbruhler September 29, 2023 10:12

pawel-peksa suggested changes Sep 29, 2023

View reviewed changes

add task solution

5acf45c

szymonpachucki requested a review from pawel-peksa September 29, 2023 14:42

choeqq approved these changes Oct 2, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Everything works outside of escape. Would love some advice on that. #772

Everything works outside of escape. Would love some advice on that. #772

szymonpachucki commented Sep 20, 2023

domalewski left a comment

domalewski Sep 24, 2023

domalewski Sep 24, 2023

domalewski left a comment

choeqq left a comment

choeqq Sep 27, 2023

choeqq Sep 27, 2023

choeqq Sep 27, 2023

choeqq Sep 27, 2023

choeqq Sep 27, 2023

choeqq Sep 27, 2023

mbruhler left a comment

mbruhler Sep 27, 2023

mbruhler Sep 27, 2023

mbruhler Sep 27, 2023

mbruhler Sep 27, 2023

pawel-peksa left a comment

pawel-peksa Sep 29, 2023

choeqq left a comment

	{visibleTodos().map((todo) => (
	{visibleTodos.map((todo) => (

		@@ -0,0 +1 @@
		export type FilterOption = 'All' \| 'Active' \| 'Completed';

Everything works outside of escape. Would love some advice on that. #772

Are you sure you want to change the base?

Everything works outside of escape. Would love some advice on that. #772

Conversation

szymonpachucki commented Sep 20, 2023

domalewski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domalewski left a comment

Choose a reason for hiding this comment

choeqq 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

mbruhler 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

pawel-peksa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

choeqq left a comment

Choose a reason for hiding this comment