Refactor Graph.defaultProps.id code into function #381
Conversation
src/components/Graph.react.js
Outdated
| .toString(36) | ||
| .substring(2, 7), | ||
| /* eslint-enable no-magic-numbers */ | ||
| id: generateId(), |
There was a problem hiding this comment.
What about multiple graphs in the same page? As-is they will all have the same id. Similar to when this was added to the dash-table.
src/components/Graph.react.js
Outdated
| @@ -69,6 +69,19 @@ export default class PlotlyGraph extends Component { | |||
| super(props); | |||
| this.bindEvents = this.bindEvents.bind(this); | |||
| this._hasPlotted = false; | |||
|
|
|||
| this.props.id = this.props.id ? this.props.id : this.generateId(); | |||
There was a problem hiding this comment.
This looks wrong. A component should never override its props.
If using state, setProps or another approach brakes tests, maybe we need to dig and find if that is correct or incorrect behavior from either the test or sub-components.
There was a problem hiding this comment.
I agree that it's wrong.
However, seeing as the test do indeed break, I'm not sure how to go from here. There are no sub-components, and the tests seem correct. Any help would be appreciated!
src/components/Graph.react.js
Outdated
| JSON.stringify(this.props.style) !== JSON.stringify(nextProps.style) | ||
| ); | ||
| } | ||
|
|
||
| componentWillReceiveProps(nextProps) { | ||
| const idChanged = this.props.id !== nextProps.id; | ||
| const idChanged = this.state.id !== nextProps.id; |
There was a problem hiding this comment.
Since we pull the id from the state everywhere now, shouldn't we update the state to the new props.id value?
|
Does the current code cause duplicate IDs (since it's in |
|
@chriddyp You're right. @valentijnnieman There's a id: generatedId() again in |
The code for generating a random id in the
Graphcomponent'sdefaultPropswas inline - causing trouble for R component generation. This PR factors the code out into it's own function, so that it is a little bit neater.