Added week2 homework

[neveenatik]

Thanks for review, and sorry that I delivered just now.
I forgot to make a PR.

[neveenatik]

State is saved on local storage but I need to figure out how I can make class name update from local storage on reload.

[prof-xed]

Hey @neveenatik , I see your Hard efforts, I think that you can improve better and better 💪.

You have a very nice files separating, and you still missing some concepts to understand yet.
so I left some comments about the main things you may make an attention on, and may help you going out through this, I dropped out any thing that may not really arrive to the main point we want to arrive.

I am highly encourage you to make an attention to these comments if something you didn't understand we feel free to ask 🙂.

[prof-xed]

instead of this you can just say targetItem.done ? // rest of the code

[prof-xed]

You have a very nice way of updating the state here, But you are modifying the state immediately, you can test this instead of yours and you will find out that you are doing it:

let newState = Object.assign({}, this.state);
const indexOfItem = newState.items.findIndex(item => item.id === id);

console.log(this.state.items[indexOfItem].done); // this will show you the previous state
let targetItem = newState.items[indexOfItem];
if (indexOfItem >= 0) {
  targetItem.done 
    ? (targetItem.done = false)
    : (targetItem.done = true);
}
console.log(this.state.items[indexOfItem].done); // this will show you the next state before setting it !!

this.setState({ items: newState.items }, () => {
  console.log(this.state.items[indexOfItem].done); // this should have the next state
});

So it would be better to use for a .map method here that doing all of these for you and it's just more readable for others.
So you may make it as

this.setState({
  list: this.state.list.map(item => {
    if (item[id] === id ){
      // return a new Object that contains the new data we need
      return {
        ...item, // using the spread operator we will copy the elements inside of `item` and replace the `done` with a new value
       done: !item.done
      }
    }
    return item
  })
});

[neveenatik]

Hi Jawhar,
Thank you very much for your feedback. It is a very useful tip.

[neveenatik]

I didn't understand how I was setting the state directly since I am manipulating the new instance of it ?

[prof-xed]

Sorry being late, I didn't saw this reply till the day.
yes you are mutating the state; because you have in line 19 a, Object.assign which in your case is just assigning a new reference to the old one.

according to mdn

The Object.assign() method is used to copy the values of all enumerable own properties from one or more source objects to a target object. It will return the target object.

in this case it's only copying the values of the Object as a references 💔.
what you may do for a deeper copy is by initializing a new source Object in the third parameter so it will be a complete new Object:

const setState = Object.assign({}, this.state, {/* the object with the new values */})

Js is wired sometimes I have no 100% correct explanation for what is happening here more than What I said.

[prof-xed]

this is only for localStorage as I see, so you may use componentWillMount instead.

[neveenatik]

That's right I am saving changes to localStorage as well so on refresh state is not reset to initial state. As far as I know componentWillMount depreciated in react 16.3 and will be removed the next major version.
Do you think then it is better maybe to set state in the constructor it is called before componentWillMount right?
I read this interesting article.
https://daveceddia.com/where-fetch-data-componentwillmount-vs-componentdidmount/
please let me know what do you think.

[prof-xed]

yes I see Thanks for update.
using constructor would make your code a bit harder to read and nothing wrong with it since it's a class base thing, and you should bind the methods that are in the component 🙃 .
but also I found another solution that may be useful

[prof-xed]

you can only say if (itemsCopy) {/* Do something */}

[prof-xed]

By looking to the mdn documentation we will find underneath the example table that the ul tag doesn't accept a div tag as a child and that may have some unexpected behaviors in the term of not following the generic rules.

[prof-xed]

No need for this key.

you are not iterating to repeat this line inside of an array to be sure that you should have it.

and there is another place you have un-needed key you should remove, if you get the Idea of it 🙂.

[prof-xed]

you can just say done ? "checked" : "unchecked"

[neveenatik]

Thanks for your review @JawharBairakdar I made all needed changes.

[prof-xed]

Nice Work 💯.

you did a great job submitting the state review.
it was all about it so I am not going to ask for any further changes.
plus I see your Great vision on the React Updates 🎉.