Skip to content

Todos design#1

Open
ofir6283 wants to merge 10 commits into
mainfrom
todosDesign
Open

Todos design#1
ofir6283 wants to merge 10 commits into
mainfrom
todosDesign

Conversation

@ofir6283

Copy link
Copy Markdown
Owner

No description provided.

@Amit1Paz Amit1Paz left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

node_modules should be in .gitignore.

Overall great job!!!
Please let me know when you are done fixing everything.

Comment thread src/App.css Outdated
Comment on lines +54 to +56
border-radius: 3em;
color: white;
padding: 15px 32px;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Do you know the difference between px, em, and rem?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Don't know the exact difference between rem and em. just the fact that they're CSS relative unit. px isn't relative.

Comment thread src/App.js Outdated
import MainBackground from './images/oceanBackground.jpg';
import TodoList from './TodoList';

// const LOCAL_STORAGE_KEY = 'todoApp.todoList';

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Commented code shouldn't be uploaded to GitHub

Comment thread src/App.js Outdated

function handleAddTodo(event) {
const name = todoNameRef.current.value
if (name === '') return

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good job thinking about form validation. Is there a difference between writing if (name === '') and if (!name)? Which one do you think is better?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Probably !name because it covers more "edge cases"

Comment thread src/App.js
Comment on lines +24 to +26
const newTodos = [...todoList];
const todo = newTodos.find(todo => todo.id === id);
todo.isCompleted = !todo.isCompleted;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can you think of another solution where you only have to Iterate over the array only once?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Maybe switching the data type from list to HashMap where the key represents the id and the value represents the task description

Comment thread src/App.js Outdated
const newTodos = todoList.filter(todo => !todo.isCompleted)
setTodos(newTodos)
}
console.log(todoList.length)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

console.log() shouldn't be uploaded to GitHub

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Bar Shoshani made me do it. I'll remove it.

Comment thread src/App.js Outdated
<div >
<div className='main-container'>

{/* <img className='main-background-img' src={MainBackground} /> */}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Commented code shouldn't be uploaded to GitHub

Comment thread src/App.js
Comment on lines +37 to +52
<div className='body' style={{ backgroundImage: `url(${MainBackground})` }}>
<div >
<div className='main-container'>

{/* <img className='main-background-img' src={MainBackground} /> */}
<input className='task-input-textbox' ref={todoNameRef} type='text' placeholder='Please Write your task here' />
<button className='add-task-btn' onClick={handleAddTodo}>Add Task</button>
<button className='completed-task-btn' onClick={handleClearTodos}>Mark Task as completed</button>
<div className='tasks-left'>Tasks Left: {todoList.filter(todo => !todo.isCompleted).length}</div>
</div>
</div>

<div className='tasks'>
<TodoList todosList={todoList} toggleTodo={toggleTodo} />
</div>
</div>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There is a lot of code here. Do you think it can be divided into different components?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Yes, but I can't think of an idea how re organized it. I would appreciate an example.

Comment thread src/App.js
<input className='task-input-textbox' ref={todoNameRef} type='text' placeholder='Please Write your task here' />
<button className='add-task-btn' onClick={handleAddTodo}>Add Task</button>
<button className='completed-task-btn' onClick={handleClearTodos}>Mark Task as completed</button>
<div className='tasks-left'>Tasks Left: {todoList.filter(todo => !todo.isCompleted).length}</div>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

div is not the correct element to use in this case. What do you think would be a better option?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Maybe using a specific component for the "tasks-left"?

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