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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
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
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
48 changes: 48 additions & 0 deletions client/app/visualizations/chart/Renderer/CustomPlotlyChart.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import React, { useState, useEffect, useMemo } from 'react';
import { RendererPropTypes } from '@/visualizations';

import { clientConfig } from '@/services/auth';
import resizeObserver from '@/services/resizeObserver';

import getChartData from '../getChartData';
import { Plotly, prepareCustomChartData, createCustomChartRenderer } from '../plotly';

export default function CustomPlotlyChart({ options, data }) {
if (!clientConfig.allowCustomJSVisualizations) {
return null;
}

const [container, setContainer] = useState(null);

const renderCustomChart = useMemo(
() => createCustomChartRenderer(options.customCode, options.enableConsoleLogs),
[options.customCode, options.enableConsoleLogs],
);

const plotlyData = useMemo(
() => prepareCustomChartData(getChartData(data.rows, options)),
[options, data],
);

useEffect(() => {
if (container) {
const unwatch = resizeObserver(container, () => {
// Clear existing data with blank data for succeeding codeCall adds data to existing plot.
Plotly.purge(container);
renderCustomChart(plotlyData.x, plotlyData.ys, container, Plotly);
});
return unwatch;
}
}, [container, plotlyData]);

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

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

CustomPlotlyChart.propTypes = RendererPropTypes;
51 changes: 51 additions & 0 deletions client/app/visualizations/chart/Renderer/PlotlyChart.jsx
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
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;
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
.plotly-chart-container {
.chart-visualization-container {
height: 400px;
overflow: hidden;
}
2 changes: 1 addition & 1 deletion client/app/visualizations/chart/chart-editor.html
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@
<div ng-if="$ctrl.options.globalSeriesType == 'custom'">
<div class="form-group">
<label class="control-label">Custom code</label>
<textarea ng-model="$ctrl.options.customCode" class="form-control v-resizable" rows="10">
<textarea ng-model="$ctrl.options.customCode" ng-model-options="{ debounce: 300 }" class="form-control v-resizable" rows="10">
</textarea>
</div>

Expand Down
6 changes: 0 additions & 6 deletions client/app/visualizations/chart/chart.html

This file was deleted.

44 changes: 17 additions & 27 deletions client/app/visualizations/chart/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,10 @@ import { registerVisualization } from '@/visualizations';
import { clientConfig } from '@/services/auth';
import ColorPalette from '@/visualizations/ColorPalette';
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 @@ -71,26 +72,6 @@ function initEditorForm(options, columns) {
return result;
}

const ChartRenderer = {
template,
bindings: {
data: '<',
options: '<',
},
controller($scope) {
this.chartSeries = [];

const update = () => {
if (this.data) {
this.chartSeries = getChartData(this.data.rows, this.options);
}
};

$scope.$watch('$ctrl.data', update);
$scope.$watch('$ctrl.options', update, true);
},
};

const ChartEditor = {
template: editorTemplate,
bindings: {
Expand Down Expand Up @@ -306,19 +287,28 @@ const ChartEditor = {
};

export default function init(ngModule) {
ngModule.component('chartRenderer', ChartRenderer);
ngModule.component('chartEditor', ChartEditor);

ngModule.run(($injector) => {
registerVisualization({
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
40 changes: 40 additions & 0 deletions client/app/visualizations/chart/plotly/customChartUtils.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import { each } from 'lodash';
import { normalizeValue } from './utils';

export function prepareCustomChartData(series) {
const x = [];
const ys = {};

each(series, ({ name, data }) => {
ys[name] = [];
each(data, (point) => {
x.push(normalizeValue(point.x));
ys[name].push(normalizeValue(point.y));
});
});

return { x, ys };
}

export function createCustomChartRenderer(code, logErrorsToConsole = false) {
// Create a function from custom code; catch syntax errors
let render = () => {};
try {
render = new Function('x, ys, element, Plotly', code); // eslint-disable-line no-new-func
} catch (err) {
if (logErrorsToConsole) {
console.log(`Error while executing custom graph: ${err}`); // eslint-disable-line no-console
}
}

// Return function that will invoke custom code; catch runtime errors
return (x, ys, element, Plotly) => {
try {
render(x, ys, element, Plotly);
} catch (err) {
if (logErrorsToConsole) {
console.log(`Error while executing custom graph: ${err}`); // eslint-disable-line no-console
}
}
};
}
Loading