Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate Chart visualization to React Part 1: Renderer #4130

Merged
merged 4 commits into from
Sep 12, 2019
Merged

Migrate Chart visualization to React Part 1: Renderer #4130

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
73c2731
Migrate Chart visualization: Renderer
kravets-levko Sep 9, 2019
ee47078
Refine PlotlyChart component; move stylesheets to visualization's folder
kravets-levko Sep 10, 2019
986aaad
Migrate Custom JS Chart to React
kravets-levko Sep 10, 2019
308104d
Cleanup
kravets-levko Sep 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion client/app/assets/less/main.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@
@import 'inc/visualizations/sankey';
@import 'inc/visualizations/pivot-table';
@import 'inc/visualizations/map';
@import 'inc/visualizations/chart';
@import 'inc/visualizations/sunburst';
@import 'inc/visualizations/cohort';
@import 'inc/visualizations/misc';
Expand Down
2 changes: 1 addition & 1 deletion client/app/pages/dashboards/dashboard.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@
.map-visualization-container,
.word-cloud-visualization-container,
.box-plot-deprecated-visualization-container,
.plotly-chart-container {
.chart-visualization-container {
position: absolute;
left: 0;
top: 0;
Expand Down
1 change: 1 addition & 0 deletions client/app/visualizations/EditVisualizationDialog.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ function EditVisualizationDialog({ dialog, visualization, query, queryResult })
options={options}
visualizationName={name}
onOptionsChange={onOptionsChanged}
context="query"
/>
</div>
</Grid.Col>
Expand Down
8 changes: 8 additions & 0 deletions client/app/visualizations/chart/Renderer/CustomPlotlyChart.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import React from 'react';
import { RendererPropTypes } from '@/visualizations';

export default function CustomPlotlyChart() {
return <div>Custom Plotly Chart</div>;
}

CustomPlotlyChart.propTypes = RendererPropTypes;
51 changes: 51 additions & 0 deletions client/app/visualizations/chart/Renderer/PlotlyChart.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import { isArray, isObject } from 'lodash';
import React, { useState, useEffect } from 'react';
import { RendererPropTypes } from '@/visualizations';
import resizeObserver from '@/services/resizeObserver';

import getChartData from '../getChartData';
import { Plotly, prepareData, prepareLayout, updateData, applyLayoutFixes } from '../plotly';

export default function PlotlyChart({ options, data }) {
const [container, setContainer] = useState(null);

useEffect(() => {
if (container) {
const plotlyOptions = { showLink: false, displaylogo: false };

const chartData = getChartData(data.rows, options);
const plotlyData = prepareData(chartData, options);
const plotlyLayout = prepareLayout(container, options, plotlyData);

// It will auto-purge previous graph
Plotly.newPlot(container, plotlyData, plotlyLayout, plotlyOptions).then(() => {
applyLayoutFixes(container, plotlyLayout, (e, u) => Plotly.relayout(e, u));
});
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arikfr That's actually all we need from Plotly here + few Plotly.relayout calls below. That's why I think we don't need extra dependency of react-plotly.js - it will not allow us to get rid of plotly.jsanyway because React component is just a wrapper around it.


container.on('plotly_restyle', (updates) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be moved to the below useEffect()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand your point. It uses some variables from the same scope, so cannot be moved to another useEffect without moving that variables to upper scope. If you mean that both useEffect calls should be merged - probably yes, but current variant is a sort of optimization: first useEffect will re-build chart when container, options and/or data changes - Plotly.newPlot will purge old chart if needed. And we need to call Plotly.purge only when container changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It uses some variables from the same scope

Perhaps one of us is misunderstanding useEffect's 2nd argument usage.
AFAIK, it's used as "componentshouldupdate". You can use any variables in the useEffect scope without adding them to the 2nd argument. Am I wrong about this?

My concern here is that container.on is being re-declared needlessly every time options and data change, but perhaps needs to only when container does.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

container.on should be called after each Plotly.newPlot because Plotly does a sort of shitty thing: it extends DOM element which is a plot container and adds new properties and methods to it, like on. So when you call Plotly.newPlot - it will destroy previous plot and unbind all events, and then you have to re-bind everything again. If that was a regular DOM events (addEventListener) - then yes, I totally agree with you

// This event is triggered if some plotly data/layout has changed.
// We need to catch only changes of traces visibility to update stacking
if (isArray(updates) && isObject(updates[0]) && updates[0].visible) {
updateData(plotlyData, options);
Plotly.relayout(container, plotlyLayout);
}
});

const unwatch = resizeObserver(container, () => {
applyLayoutFixes(container, plotlyLayout, (e, u) => Plotly.relayout(e, u));
});
return unwatch;
}
}, [options, data, container]);

// Cleanup when component destroyed
useEffect(() => {
if (container) {
return () => Plotly.purge(container);
}
}, [container]);

return <div className="chart-visualization-container" ref={setContainer} />;
}

PlotlyChart.propTypes = RendererPropTypes;
16 changes: 16 additions & 0 deletions client/app/visualizations/chart/Renderer/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import React from 'react';
import { RendererPropTypes } from '@/visualizations';

import PlotlyChart from './PlotlyChart';
import CustomPlotlyChart from './CustomPlotlyChart';

import './renderer.less';

export default function Renderer({ options, ...props }) {
if (options.globalSeriesType === 'custom') {
return <CustomPlotlyChart options={options} {...props} />;
}
return <PlotlyChart options={options} {...props} />;
}

Renderer.propTypes = RendererPropTypes;
2 changes: 1 addition & 1 deletion ...assets/less/inc/visualizations/chart.less → ...sualizations/chart/Renderer/renderer.less
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
.plotly-chart-container {
.chart-visualization-container {
height: 400px;
overflow: hidden;
}
22 changes: 17 additions & 5 deletions client/app/visualizations/chart/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import getChartData from './getChartData';
import template from './chart.html';
import editorTemplate from './chart-editor.html';

import Renderer from './Renderer';

const DEFAULT_OPTIONS = {
globalSeriesType: 'column',
sortX: true,
Expand Down Expand Up @@ -314,11 +316,21 @@ export default function init(ngModule) {
type: 'CHART',
name: 'Chart',
isDefault: true,
getOptions: options => merge({}, DEFAULT_OPTIONS, {
showDataLabels: options.globalSeriesType === 'pie',
dateTimeFormat: clientConfig.dateTimeFormat,
}, options),
Renderer: angular2react('chartRenderer', ChartRenderer, $injector),
getOptions: (options) => {
const result = merge({}, DEFAULT_OPTIONS, {
showDataLabels: options.globalSeriesType === 'pie',
dateTimeFormat: clientConfig.dateTimeFormat,
}, options);

// Backward compatibility
if (['normal', 'percent'].indexOf(result.series.stacking) >= 0) {
result.series.percentValues = result.series.stacking === 'percent';
result.series.stacking = 'stack';
}

return result;
},
Renderer,
Editor: angular2react('chartEditor', ChartEditor, $injector),

defaultColumns: 3,
Expand Down
14 changes: 11 additions & 3 deletions client/app/visualizations/chart/plotly/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,20 +14,28 @@ import prepareLayout from './prepareLayout';
import updateData from './updateData';
import applyLayoutFixes from './applyLayoutFixes';

export {
Plotly,
prepareData,
prepareLayout,
updateData,
applyLayoutFixes,
};

Plotly.register([bar, pie, histogram, box, heatmap]);
Plotly.setPlotConfig({
modeBarButtonsToRemove: ['sendDataToCloud'],
});

const PlotlyChart = () => ({
restrict: 'E',
template: '<div class="plotly-chart-container" resize-event="handleResize()"></div>',
template: '<div class="chart-visualization-container" resize-event="handleResize()"></div>',
scope: {
options: '=',
series: '=',
},
link(scope, element) {
const plotlyElement = element[0].querySelector('.plotly-chart-container');
const plotlyElement = element[0].querySelector('.chart-visualization-container');
const plotlyOptions = { showLink: false, displaylogo: false };
let layout = {};
let data = [];
Expand Down Expand Up @@ -77,7 +85,7 @@ const PlotlyChart = () => ({

const CustomPlotlyChart = clientConfig => ({
restrict: 'E',
template: '<div class="plotly-chart-container" resize-event="handleResize()"></div>',
template: '<div class="chart-visualization-container" resize-event="handleResize()"></div>',
scope: {
series: '=',
options: '=',
Expand Down