Initial add of React TodoMVC to labs

[petehunt]

React is a JavaScript library for building user interfaces by Facebook and Instagram. It powers many components on Facebook.com and all of Instagram.com is written with it. We have two TodoMVC examples checked into our repo: this one, which has no dependencies, and another one which showcases Backbone integration. I've only included the first one for now, if you guys think it's a good idea I can put out a PR for the one with Backbone integration.

I read the contributing guide and I think this meets the standards, let me know if I missed something.

[sindresorhus]

Don't change

[petehunt]

Cool, made this fix. Also fixed an issue with spaces instead of tabs in the index.html file.

[passy]

Thanks for the submission, @petehunt! A few notes:

• Can you take a look at the style guide? Running jshint against js/ folder can make this a lot easier.
• We prefer not to have external dependencies, so the project can be used offline. Preferably integrated as bower components.
• Blurring an input or pressing escape should exit the edit state.
• Items should be trim()ed before saved.

[addyosmani]

Thanks for the submission, Pete. I was actually quite interested in looking at the Backbone integration example to see how it compared to other extension frameworks. Would it be possible to get another labs PR for it?

As @passy mentioned above, there are a few more changes we would love to land in order for it to meet our specs, but as this is a labs submission it really depends on how soon you think you might be able to get around to making the updates.

[petehunt]

Sure, I figured I'd land this one first so the second one could go more smoothly. I'll take a pass at it in the morning as I'm totally beat from the launch today, but if you'd like to take a peek at our Backbone integration it's here: https://github.com/facebook/react/blob/master/examples/todomvc-backbone/js/app.js

[petehunt]

@passy It's pretty hard for me to get this to pass jshint because when I desugar jsx syntax, the desugared code doesn't pass it (superficial things, like whitespace). I think I'm pretty close to the coding standards now, though.

[passy]

@petehunt Oops, didn't even think about that. Thanks for the update! I'm going to take a closer look today and leave comments at the relevant places.

[petehunt]

Another option is I could desugar the jsx and check in raw JavaScript that meets the guidelines, but I don't think that's in the spirit of TodoMVC since the preferred way to write React right now is to use JSX (though you don't need to use it if you don't want to).

Sounds like this approach is at least good enough for labs, but let me know if you want me to change it up.

[passy]

@petehunt Yes, we'd definitely prefer a version that you would consider an example of the framework's best practices.

[stephenplusplus]

Hey @petehunt, I went through the app to try to accomplish a few things:

• General, small code style stuff
• Bring in the todomvc-common Bower component. This includes the updated base.css and base.js files.
  • I shortly realized React takes control of ids and nullifies most of our base.css file, so I included your css file as app.css (as opposed to base.css to avoid confusion), and kept the class overrides in there.
• Split the JS into 4 total pieces (any better suggestions are welcome!):
  • js/utils.js
  • js/todoItem.js
  • js/footer.js
  • js/app.js

Check it out and let me know if I broke any React principles in the process, since we don't want that!

[petehunt]

@stephenplusplus that looks great! A few things (my bugs to begin with):

• You could move this https://github.com/stephenplusplus/todomvc/blob/react/labs/architecture-examples/react/js/app.js#L87 to a new method called componentDidUpdate()
• In my Backbone version (https://github.com/facebook/react/blob/master/examples/todomvc-backbone/js/app.js) I made this.state.editing a scalar instead of an object which makes more sense since only one TODO item may be edited at a time.

But those aren't real bugs and aren't too bad. What may be easiest is if you committed that, and I could put out a pull immediately after to clean those up.

[stephenplusplus]

Glad to hear I didn't mess anything up! :)

Instead of leaving the trail in this repo's history, it would work just as nicely if you just did a...

git remote add stephenplusplus [email protected]:stephenplusplus/todomvc.git
git fetch stephenplusplus
git checkout -b react stephenplusplus/react

Then you can edit away, squash, and force push to petehunt/react to update this PR.

[addyosmani]

Ping :) would be great to keep the momentum going on this implementation.

[petehunt]

My bad! Pushed @stephenplusplus's version with a few minor tweaks.

This is what happens when I don't put something in an issue tracker :P

[stephenplusplus]

@petehunt last thing is just squash 'em down to 1 commit, please :)

[petehunt]

Oops, should be good now :) Thanks for being patient!

[stephenplusplus]

Thank you as well for being so patient! :)

I did run into a few issues though.

1. Please go through the files in the js/ directory and convert any space indentation to tabs. In js/utils.js, it's all spaces, however in js/todoItem.js, there is just one occurrence of space indentation.
2. When all items are marked as complete or incomplete, the .toggle-all checkbox should be updated to reflect the status as well.
3. Double clicking to edit a todo item should place the cursor at the end of the value. See the AngularJS app as an example.
4. While in edit mode, if the input box is left empty, the blur/submit event should discard the todo item.

If you need any clarification, please let me know!

[petehunt]

Cool. I will try to finish this up this weekend. Thanks again

[petehunt]

OK. I addressed your comments. I also re-ran through all the behaviors in https://github.com/tastejs/todomvc/wiki/App-Specification and fixed the escape key behavior. This should have everything but routing, which React doesn't really want to solve (better to delegate to a tool that's solved it thoroughly, imo).

What are people's thoughts on Selenium integration? I poked around but it seemed like there was tons and tons of discussion but not necessarily a conclusion. If I can find some time I may take a shot at it, but I want to make sure it's something that people want.

[passy]

What are people's thoughts on Selenium integration? I poked around but it seemed like there was tons and tons of discussion but not necessarily a conclusion. If I can find some time I may take a shot at it, but I want to make sure it's something that people want.

Any help on the testing topic would be greatly appreciated. We still haven't found a good solution for end to end testing, but if you can provide an example of how to do that with selenium - specific to the React example or generic -, that would be fantastic.

[addyosmani]

Side: I wonder if it's worth us punting on Selenium integration for TodoMVC in favor of doing this instead for TasteApp (our next, more complex version of this project) (tastejs/TasteApp#1). As it's a completely fresh application we may be able to design it from the ground up with such integration in mind. I personally wouldn't mind better testing for TodoMVC regardless if someone was interested in exploring however :)

Great work once again on addressing the issues above, @petehunt!

[passy]

Two things I found:

• When you edit an existing entry, the value isn't trimmed.
• There's an interesting bug I can't explain: If you have a list of todos and delete on in the middle of the list and double-click on one that moved up, the content in the input element is still from the deleted element. I hope you can reproduce it from this description.

[petehunt]

Added trimming and fixed the input element bug. That's a React gotcha that will be fixed in our upcoming release but I corrected it in the latest update.

[passy]

I hope this isn't some caching issue on my end, but when I add a new todo, edit it and add some spaces to the end, save it and enter edit mode again, I still see those spaces. They don't seem to be persisted, though.

[petehunt]

Hmm really? I just tested that behavior and it seems to work. What browser?

[passy]

@petehunt Chrome 27.0.1453.110

[petehunt]

@passy I can't repro with that Chrome version or Firefox (both mac). I know a common configuration is to run python -m SimpleHTTPServer. I've had issues with Chrome wanting to cache that stuff indefinitely sometimes. I wonder if that's an issue.

[passy]

Just tried freshly checked this out on my desktop computer and everything works as it should. This must indeed had been some caching issue.

Thanks so much for the hard work, @petehunt. And thanks for the support, @stephenplusplus. 🍻

[petehunt]

Thank you guys for helping out and being patient! Big fan of what you're doing here. Specifically I think it's really great that you're valuing the little details that tend to be overlooked in simple examples but bite you in real apps :)

Will chew over the testing stuff and hopefully spin up a conversation with you guys. What would be the best medium for that?

[passy]

Will chew over the testing stuff and hopefully spin up a conversation with you guys. What would be the best medium for that?

If you have some code to show, a pull request is probably best. If you just need a little input, you can ping all of us on Twitter or GTalk. :)

[sindresorhus]

Thanks this addition @petehunt :)