Skip to content

Conversation

sophiebits
Copy link
Collaborator

Instead, use .concat and make a new todos array.

(First react pull request, lemme know if I did something wrong.)

@zpao
Copy link
Member

zpao commented May 31, 2013

These look good to me - I think this better conveys the idea that modifying this.state isn't great. Effectively they're the same since we call setState anyway.

It looks like @petehunt actually modified these examples since you pulled, so it's probably worth rebasing to make sure you're working from the latest copy

Instead, use .concat and make a new todos array.

Test Plan:
Added todo items successfully.
@sophiebits
Copy link
Collaborator Author

Indeed -- just rebased.

@petehunt
Copy link
Contributor

Yep this is definitely better. This was a hackathon project and still shows in some places :P

petehunt added a commit that referenced this pull request May 31, 2013
Change TodoMVC to not modify state in place
@petehunt petehunt merged commit bb4788e into facebook:master May 31, 2013
@sophiebits sophiebits deleted the immutable-state branch May 31, 2013 08:17
@jordwalke
Copy link
Contributor

You read my mind!

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.

4 participants