React component for plotly charts #2633
Conversation
| const Plot = createPlotlyComponent(Plotly); | ||
|
|
||
|
|
||
| const timeSeriesToPlotlySeries = (ss) => { |
There was a problem hiding this comment.
FWIW the parens are optional when there is only one variable. This could be rewritten as:
const timeSeriesToPlotlySeries = ss => {There are also many places below where this comment applies, like line 24 and line 26. But it's obviously just style, and it's up to you.
There was a problem hiding this comment.
Curious. eslint has caught this many times for me, not sure how I missed this.
There was a problem hiding this comment.
-- Ah. the arrow-parens eslint rule expects parens for braceful arrow functions and no parens for braceless ones.
| x: null, | ||
| ys: null, | ||
| }; | ||
| this.refreshCustom = this.refreshCustom.bind(this); |
There was a problem hiding this comment.
There's a neat way around this that I recently discovered myself. You can omit this line if you define refreshCustom like this:
refreshCustom = (figure, plotlyElement) => {
// ...
}See this blog post section for more info. As with everything, there are pros and cons with this approach, but if you find that you have a lot of this.whatever = this.whatever.bind(this) lines in your code, it's a trick worth considering.
| if (!nextProps.options) { return {}; } | ||
| if (nextProps.options.globalSeriesType === 'custom') { | ||
| const [x, ys] = timeSeriesToPlotlySeries(nextProps.series); | ||
| return { x, ys, revision: prevState.revision + 1 }; |
There was a problem hiding this comment.
Nice use of ES6 default prop values! 😃
| } catch (err) { | ||
| if (this.props.options.enableConsoleLogs) { | ||
| // eslint-disable-next-line no-console | ||
| console.log(`Error while executing custom graph: ${err}`); |
There was a problem hiding this comment.
Nice use of template strings! 😃
| } | ||
|
|
||
| render() { | ||
| if (!this.props.options) { return ''; } |
There was a problem hiding this comment.
By convention, I try to return null when I don't want a render function to do anything. I don't think anything weird will happen if you return an empty string, but I guess it could theoretically have some weird side effects.
There was a problem hiding this comment.
Also, if your ESLint rules allow it, this could be rewritten as:
if (!this.props.options) return '';or
if (!this.props.options) return null;| className="plotly-chart-container" | ||
| revision={this.state.revision} | ||
| style={{ width: '100%', height: '100%' }} | ||
| useResizeHandler |
There was a problem hiding this comment.
Oh wow, I didn't even know you could do this. Nice find!
| } | ||
|
|
||
| static getDerivedStateFromProps(nextProps, prevState) { | ||
| if (!nextProps.options) { return {}; } |
There was a problem hiding this comment.
getDerivedStateFromProps should return null if state doesn't need to be updated.
|
Thanks @openjck, you caught some stuff I learned about later but never came back and applied to this earlier code :-) |
|
I know this won't be ready to merge until after the 5.0 release, but if you'd have time to look it over beforehand I'd appreciate it. |
I'm not sure who that's addressed to. If it would help, I'd be happy to re-review. |
|
@washort The tests are failing in the build stage, I tried to rebuild them a few times but it's not a transient error. |
|
This is superseded by #2988. |
This uses the official Plotly React components to do charts.
I'm not sure if this has compatibility issues with existing custom charts, since we don't use them.