
A React.js case study - xngzng
http://blog.krawaller.se/posts/a-react-js-case-study/
======
wingspan
Thanks for putting this together, always interesting to see people use React
for the first time! I did a code review of sorts and cleaned up the code to be
more idiomatic React. You can find the series of commits with in-depth
comments on GitHub [1]. I may even write this up as a blog post on my website
[2].

In short:

\- Don't pass around components and call methods on them; prefer to pass props
instead

\- Conditional rendering instead of hiding, exactly as [3]

\- Use the JSX harmony transforms

\- Declare props

\- Use classSet

\- setState only needs to include props that change, rest stay the same

[1]
[https://github.com/ianobermiller/reactexperiment/commits/](https://github.com/ianobermiller/reactexperiment/commits/)

[2] [http://ianobermiller.com](http://ianobermiller.com)

[3]
[https://news.ycombinator.com/item?id=8247422](https://news.ycombinator.com/item?id=8247422)

~~~
lobster_johnson
I agree with all of these changes. Code-style-wise, I prefer to call the
actual handler implementations `handleFoo` as opposed to `onFoo`. It's a very
small detail, but it makes code a little easier to sift through (as well as
search/replace), because you always know that `onFoo` is a prop (whose
implementation is foreign to the component) and `handleFoo` is a method (whose
implementation is native to the component).

~~~
wingspan
Not a bad idea. We would typically use _onClick for any component handler
methods, so the _ would distinguish it. We have an internal transform that
mangles anything in a module starting with _ so that it cannot be accessed
outside.

~~~
lobster_johnson
I also prefix with _, if only to indicate what's private and what's API. Mind
sharing your transform?

------
lobster_johnson
> I first tried to do this: [...] ...which actually worked, but generated a
> React error message about an unmounted component.

I suspect that you were doing something wrong that triggered this error.
Unmounting components based on state is perfectly idiomatic React, and I do it
all the time:

    
    
        {this.state.mode === 'map' && <Map/>}
    

You pay for the added DOM work, of course, but this is usually negligible in
my experience, especially in your case. You do have to make sure you follow
the component lifecycle so that you don't try to use getDOMNode() or similar
with a component that is not currently mounted.

~~~
_RPM
How does React work under the hood. JavaScript would normally throw an error
if it found <Map /> in the source code. How does React do this? Is it JS?

~~~
hcarvalhoalves
This is not React's, but a javascript preprocessor called JSX that compiles to
JS (like LESS -> CSS).

[http://facebook.github.io/react/docs/jsx-in-
depth.html](http://facebook.github.io/react/docs/jsx-in-depth.html)

Something like <Map foo={bar}/> would compile to a function call Map({foo:
bar}). Behind the scenes, React is not dealing with HTML.

You can use React without JSX, but it's useful.

~~~
_RPM
Is React a client-side framework, or server-side?

~~~
baddox
It's primarily client-side, but React components output a virtual
representation of the DOM, so it's relatively simple to render a page on the
server and return HTML.

------
rdtsc
I see some criticism here but I think it is worth pointing out this looks like
a person learning to use a new technology and sharing their experience, which
is great.

Sometimes that means using non-idiomatic constructs or just having some rough
edges. Please be considerate.

------
illicium
I'm not particularly fond of the way React mixes presentation (returning nodes
in render()) and logic. Particularly things like:

    
    
        var classes = _.reduce(["flipped","correct","wrong"],function(m,c){return m+(this.state[c]?c+" ":"");},"",this);
    

seem very inelegant compared to say, Angular's ngClass, which does the same
thing in a data-driven way. But, that's not to say Angular is free from
template logic either.

~~~
matchu
This isn't a React issue; this is a failure to use an abstraction where
appropriate. React offers the classSet plugin for exactly this use case.

[http://facebook.github.io/react/docs/class-name-
manipulation...](http://facebook.github.io/react/docs/class-name-
manipulation.html)

I think the React authors agree that React classes shouldn't contain excessive
logic; if your class does, consider extracting it to somewhere more
appropriate.

~~~
krawaller
Oh, indeed, I've completely missed the classSet plugin! That would indeed make
the code more elegant. And fair point about the logic perhaps being too
complicated here.

Thanx for the pointers!

------
ufo
Why do

    
    
        className={this.state.playing ? "hidden" : "showing"}
    

instead of

    
    
      <div>
          {(this.state.playing
               ? <Board/>
               : <Wordform/> )}
      </div>

~~~
pselbert
It is much simpler to animate between two classes in straight CSS. That's the
primary reason I favor the `className` approach.

~~~
lobster_johnson
React has a solution for that which actually automatically applies class names
as components are inserted and removed:
[http://facebook.github.io/react/docs/animation.html](http://facebook.github.io/react/docs/animation.html).
It's not perfect, and it's better than doing it manually.

------
krawaller
There's now a follow-up to this post, walking through a refactor based on the
comments given here and elsewhere. Thank you all for the feedback!

[https://news.ycombinator.com/item?id=8273026](https://news.ycombinator.com/item?id=8273026)

------
maccard
Doesn't work properly for me using FF 31.0

I can see the words on the tiles before clicking them.

~~~
krawaller
Might be I've been sloppy with adding -moz- to some css rules. Will take a
look!

~~~
pkroll
Same, I can see the words (backwards) before clicking a tile. Chrome works
fine, except it seems like after a match the first click on a tile doesn't do
anything, and I have to click again to make it flip.

~~~
krawaller
The not doing anything is intentional - only when the pair is resolved and the
status row goes back to saying "choose a tile" can you click a new tile. This
"feature" really needs to be communicated better though, sorry abouy that.

------
theycallmemorty
Off-topic: Why do people use lodash over underscore.js?

