Amos zohar #9
Amos zohar #9
Conversation
There was a problem hiding this comment.
Good job on your work, left you some comments.
Please remove your comments. the code should be self explained.
Also, comment like this :
/* Additional styles to ensure the rest of the content flows */
margin: 0;
padding: 0;Are not needed for some one who knows css.
| flex-direction: row; | ||
| justify-content: flex-start; | ||
| /* align-items:center; */ | ||
| } |
There was a problem hiding this comment.
Remove this if you are not using
| justify-content: space-around; | ||
| padding: 1vmax; | ||
| /* align-items:center; */ | ||
| } |
There was a problem hiding this comment.
Same comment in here - remove if not used
| const [tasksList, setTasksList] = useState([]); | ||
| const [filteredTasksList,setFilteredTasksList] = useState([]); | ||
| const [selected, setSelected] = useState(ALL); | ||
|
|
There was a problem hiding this comment.
Good job using constants, I think a better name for your folder would be constants because those are constants values
| const [selected, setSelected] = useState(ALL); | ||
|
|
||
| useEffect(() => setTasksList(tasks), []); | ||
| const addTask = (description) => |
There was a problem hiding this comment.
Can you explain why you used the use effect hook when your data is hardcoded? you could simply const [tasksList, setTasksList] = useState(tasks);
Was there any reason for it ?
| return !task.active; | ||
| default: | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Do you need the default here? It seems to me that the task will either be active or not. Do you have a scenario where no case matches?
| import {ALL,ACTIVE,COMPLETED} from '../mock'; | ||
| const CLEAR_COMPLETED = "Clear Completed"; | ||
| const itemsString = amount => `${amount} items left`; | ||
| const Navbar = ({filterHandler,clearHandler,counter}) => |
There was a problem hiding this comment.
Could you think of a better name for this variable?
We should understand from the name of a variable its meaning( for example itemsLeft )
|
|
||
| const tasks = [ | ||
| { | ||
| id: 1, |
There was a problem hiding this comment.
Hey, about that file name suggestion I made earlier, I think it would be better to split the mock data into its file, separate from the constants. I initially thought this file was just for constants.
For example
constants.js :
const ACTIVE = "Active";
const COMPLETED = "Completed";
const ALL = "All";mocks.js :
const tasks = [
{
.
.
]
No description provided.