Skip to content

Comments

Kendiproject1#38

Open
kendigm wants to merge 3 commits intorocketacademy:mainfrom
kendigm:main
Open

Kendiproject1#38
kendigm wants to merge 3 commits intorocketacademy:mainfrom
kendigm:main

Conversation

@kendigm
Copy link

@kendigm kendigm commented Nov 17, 2023

No description provided.

}
return todos.map((todo, index) => (
<div
className={todo.isCpmplete ? "todo-row complete" : "todo-row"}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have a typo here, I think this will not work

<div className="icons">
<RiCloseCircleLine
onClick={() => removeTodo(todo.id)}
className="delete - icon"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with the spacing in the className, this styling will probably not be applied properly

Comment on lines +9 to +10
if (props.editMode) setUpdate(e.target.value)
else setInput(e.target.value);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you use return statements, the else is redundant here.

Suggested change
if (props.editMode) setUpdate(e.target.value)
else setInput(e.target.value);
if (props.editMode) return setUpdate(e.target.value)
setInput(e.target.value);

});
const toggleColorScheme = () => {
setColorScheme(colorScheme === 'dark' ? 'light' : 'dark');
console.log(colorScheme);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove console logs before pushing your code

Comment on lines +50 to +52
// if (!newValue || /^|s*$/.test(newValue.text)) {
// return;
// }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this commented out?

};

const removeTodo = (id) => {
const removeArr = [...todos].filter((todo) => todo.id !== id);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const removeArr = [...todos].filter((todo) => todo.id !== id);
const removeArr = todos.filter((todo) => todo.id !== id);

making a copy of todos here is redundant, as filter will return a new array anyways

Comment on lines +65 to +67
if (todo.id === id) {
todo.isComplete = !todo.isComplete;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By doing this, you change the value of the original todos. See https://bootcamp.rocketacademy.co/0-foundations/0.4-javascript/0.4.3-reference-vs-value

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest making a new copy of the todo if you want to get a new array with the inverted values

};

const completeTodo = (id) => {
let updatedTodos = todos.map((todo) => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you just want to complete a single Todo, it might make more sense to use .find() here or a normal for loop, as you don't need to update every single todo in the array. Would like to know why you are using a map here

if (edit.id) {
return <TodoForm edit={edit} editMode={editMode} setEditMode={setEditMode} onSubmit={submitUpdate} />;
}
return todos.map((todo, index) => (

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is odd to render a list of todos in the Todo component. This component should define what a single todo looks like only

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the TodoList component I would do something like this:

....
return todos.map((todo, index) => <Todo todo={todo} key={index} />
....

@@ -3,8 +3,13 @@
"version": "0.1.0",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the README file is missing any content about your project. The planning documents are also missing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants