Khan / react-components

khan.github.io/react-components/
MIT License
1.01k stars 99 forks source link

Use refreshMillis prop as the interval #35

Closed jhubert closed 9 years ago

jhubert commented 9 years ago

I also a replaced the "TODO" about why using bind with an explanation of why bind matters there. Hopefully other developers will find that helpful.

ariabuckles commented 9 years ago

Thanks! I'd be happy to accept the refreshMillis fix!

I believe Joel's comment isn't referencing how bind works in general though--normally React autobinds methods on your component, so that you don't need to .bind any of them yourself. I think it doesn't autobind methods that you don't write though, like setState or forceUpdate, which is why bind was necessary here. A comment to that effect might be more helpful.

jhubert commented 9 years ago

Just created pull #36, which doesn't have the bind comment change in it. Closing this one. :smile:

joelburget commented 9 years ago

TIL :)

sophiebits commented 9 years ago

:) True. Autobinding is confusing which is why we're moving towards arrow functions on class property initializers instead.

class TimeAgo extends React.Component {
  monkey() {
    // I'm not autobound...
  }
  gorilla = () => {
    // but I am!
  }
}
jhubert commented 9 years ago

I'm probably in the minority, but I find the es6 syntax to be really confusing and chaotic. I suppose I'll have to get used to it. :disappointed:

sophiebits commented 9 years ago

@jhubert Yeah, that's a valid concern. Our hope is that it's unfamiliar now but will be familiar once you learn it elsewhere (or you'll learn it for React and then be able to reapply it elsewhere).

ariabuckles commented 9 years ago

@spicyj it's gorilla = not gorilla: ? I guess I'd expect gorilla: bind to the scope outside the class?

sophiebits commented 9 years ago

Here's the latest proposal:

https://gist.github.com/jeffmo/054df782c05639da2adb

I think the colon conflicts with type syntax. The colon/equals/comma/no-comma/semicolon thing is unfortunate though.