[DataViz] Snapshot works for GoogleCharts #33703
Conversation
apps/src/imageUtils.js
Outdated
| imageType = imageType || 'image/png'; | ||
| options = options || {}; |
There was a problem hiding this comment.
Why not use default parameters here?
| return <div ref={element => (this.chartArea = element)} />; | ||
| return ( | ||
| <div | ||
| id="googleChartContainer" |
There was a problem hiding this comment.
If we're not going to explicitly pass refs around, it might be a good idea to share this id between components with a constant, so we won't accidentally change it in only one place in the future.
| class Snapshot extends React.Component { | ||
| static propTypes = { | ||
| chartType: PropTypes.number.isRequired, |
There was a problem hiding this comment.
If you want to get really specific,
chartType: PropTypes.oneOf(Object.values(ChartType)).isRequired,| this.getImageFromCrossTab(); | ||
| break; | ||
| default: | ||
| } |
There was a problem hiding this comment.
The empty default case seems weird. Why not
if (ChartType.CROSS_TAB === this.props.chartType) {
this.getImageFromCrossTab();
} else {
this.getImageFromGoogleChart();
}...until things are more complicated?
| svgToDataURI(svg, 'image/png', {renderer: 'native'}).then(imageURI => { | ||
| if (this.isMounted_) { | ||
| this.setState({imageSrc: imageURI}); | ||
| } |
There was a problem hiding this comment.
I've heard isMounted is an antipattern but have never been totally satisfied with the alternatives - cancellable promises are cool, but also have more boilerplate than I'd like.
No need to change anything, but I'd love to hear your thoughts on this.
There was a problem hiding this comment.
Oo thanks for pointing this out. I didn't know isMounted was an anti-pattern, I think I just modeled this off of similar structures elsewhere in our codebase. I will look into cancellable promises as a follow-up PR.
| return <div ref={element => (this.chartArea = element)} />; | ||
| return ( | ||
| <div | ||
| id="googleChartContainer" |
There was a problem hiding this comment.
nit: use kebab-case for element IDs
| return; | ||
| } | ||
| svgToDataURI(svg, 'image/png', {renderer: 'native'}).then(imageURI => { | ||
| if (this.isMounted_) { |
There was a problem hiding this comment.
i don't think i understand why this.isMounted_ is necessary -- won't this method only ever be called when the component has already mounted?
There was a problem hiding this comment.
i just noticed Brad's comment that's basically the same -- feel free to answer there
There was a problem hiding this comment.
getImageFromGoogleChart will only be called when the component is mounted, but it's possible that the snapshot modal is closed before svgToDataURI finishes, in which case the component will be unmounted by the time the callback runs. That's why I have the isMounted_ check before setting the state from the callback
Part 1
Implements rendering GoogleChart types (bar, histogram, scatter plot) as
<img>elements in the snapshot modal.Bar Chart:
Histogram:
Scatter Plot:
Links
Testing story
Reviewer Checklist: