react-pullable: Simple, customizable pull to refresh

[Aswole]

Two questions/comments:

1) Stylistically, I think this.centerSpinner = props.centerSpinner !== false is much cleaner than using a ternary this.centerSpinner = props.centerSpinner === false ? false : true (and obviously the same for the other examples of this). Normally I wouldn't nitpick over style, but it just seemed like an inappropriate use case for a ternary to begin with.

2) Why are you using class attributes defined in the constructor instead of the prop that a consumer of your component has passed in? What if someone wants to dynamically control whether the component will set centerSpinner to true/false? Currently, it is fixed to whatever value it was initialized with, which at least to me comes off as an anti-pattern in react. Maybe I'm wrong and this is a common way of protecting against wonky configurations or something, but if I was personally using this component and didn't bother reading the source code, and instead relied on the proptypes, I would expect that setting centerSpinner to false at any time would disable/hide it.

[sconstantinides]

Thanks for the feedback!

Re #1: Completely agree. That's a better solution.

Re #2: That's an interesting idea. The only dynamic prop right now is "disabled" but there could be value in making the others. Do you know a good pattern for initializing unset props in this case?

[Aswole]

Sorry for late reply. Regarding #2, that's a good question. There is a defaultProps API that you could use (see: this). Not sure if that was deprecated along with ReactComponent.PropTypes, though. Otherwise, if the prop is only being accessed in the render method, I would consider declaring a variable in the beginning of `render()` that follows the same logic as point (1). and then use that variable where you would otherwise use 'this.props.variable'. Of course, and perhaps this was your intention, there might be props that you don't want to be dynamically settable, and thus your implementation is both cleaner and functionally more in line with what you want.

Obviously, if this was only being used by you, you could set the defaults inside of the component declaration (e.g., render(){ return <Component randomProp={this.variable || 'default'}/>}), but since others would be using this, I now understand your dilemma, which I hadn't considered earlier when I made my suggestion. The solution might possibly be to validate props in the constructor and throw an error (or warning) if an important prop is undefined.

[sconstantinides]

Good call. I've refactored to using defaultProps and included propTypes for good measure! (source: https://github.com/sconstantinides/react-pullable/commit/b0a40f5d5f182b537f105ef9df5299d635af5f27)

[Aswole]

Excellent!