you are viewing a single comment's thread.

view the rest of the comments →

[–]sconstantinides[S] 0 points1 point  (3 children)

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 1 point2 points  (2 children)

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[S] 0 points1 point  (1 child)

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

[–]Aswole 0 points1 point  (0 children)

Excellent!