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

Allow the user to decide how to handle null values in charts #4071

Merged
merged 14 commits into from
Sep 9, 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
9 changes: 0 additions & 9 deletions client/app/lib/value-format.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,15 +64,6 @@ export function createNumberFormatter(format) {
return value => toString(value);
}

export function createFormatter(column) {
switch (column.displayAs) {
case 'number': return createNumberFormatter(column.numberFormat);
case 'boolean': return createBooleanFormatter(column.booleanValues);
case 'datetime': return createDateTimeFormatter(column.dateTimeFormat);
default: return createTextFormatter(column.allowHTML && column.highlightLinks);
}
}

export function formatSimpleTemplate(str, data) {
if (!isString(str)) {
return '';
Expand Down
6 changes: 6 additions & 0 deletions client/app/visualizations/chart/chart-editor.html
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,12 @@
<input type="checkbox" ng-model="$ctrl.options.series.percentValues"> Normalize values to percentage
</label>
</div>

<div ng-if="['bubble', 'scatter'].indexOf($ctrl.options.globalSeriesType) === -1" class="checkbox">
<label class="control-label">
<input type="checkbox" ng-model="$ctrl.options.missingValuesAsZero"> Treat missing/null values as 0
</label>
</div>
</div>
</div>

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
{
"input": {
"data": [
{ "a": 42, "b": 10, "g": "first" },
{ "a": 62, "b": 73, "g": "first" },
{ "a": 21, "b": 82, "g": "second" },
{ "a": 85, "b": 50, "g": "first" },
{ "a": 95, "b": 32, "g": "second" }
],
"options": {
"columnMapping": {
"a": "x",
"b": "y",
"g": "series"
},
"seriesOptions": {}
}
},
"output": {
"data": [
{
"name": "first",
"type": "column",
"data": [
{ "x": 42, "y": 10, "$raw": { "a": 42, "b": 10, "g": "first" } },
{ "x": 62, "y": 73, "$raw": { "a": 62, "b": 73, "g": "first" } },
{ "x": 85, "y": 50, "$raw": { "a": 85, "b": 50, "g": "first" } }
]
},
{
"name": "second",
"type": "column",
"data": [
{ "x": 21, "y": 82, "$raw": { "a": 21, "b": 82, "g": "second" } },
{ "x": 95, "y": 32, "$raw": { "a": 95, "b": 32, "g": "second" } }
]
}
]
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
{
"input": {
"data": [
{ "a": 42, "b": 10, "c": 41, "d": 92 },
{ "a": 62, "b": 73 },
{ "a": 21, "b": null, "c": 33 },
{ "a": 85, "b": 50 },
{ "a": 95 }
],
"options": {
"columnMapping": {
"a": "x",
"b": "y",
"c": "y"
},
"seriesOptions": {}
}
},
"output": {
"data": [
{
"name": "b",
"type": "column",
"data": [
{ "x": 42, "y": 10, "$raw": { "a": 42, "b": 10, "c": 41, "d": 92 } },
{ "x": 62, "y": 73, "$raw": { "a": 62, "b": 73 } },
{ "x": 21, "y": null, "$raw": { "a": 21, "b": null, "c": 33 } },
{ "x": 85, "y": 50, "$raw": { "a": 85, "b": 50 } }
]
},
{
"name": "c",
"type": "column",
"data": [
{ "x": 42, "y": 41, "$raw": { "a": 42, "b": 10, "c": 41, "d": 92 } },
{ "x": 21, "y": 33, "$raw": { "a": 21, "b": null, "c": 33 } }
]
}
]
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
{
"input": {
"data": [
{ "a": 42, "b": 10, "g": "first" },
{ "a": 62, "b": 73, "g": "first" },
{ "a": 21, "b": 82, "g": "second" },
{ "a": 85, "b": 50, "g": "first" },
{ "a": 95, "b": 32, "g": "second" }
],
"options": {
"columnMapping": {
"a": "x",
"b": "y",
"g": "series"
},
"seriesOptions": {
"first": { "zIndex": 2 },
"second": { "zIndex": 1 }
}
}
},
"output": {
"data": [
{
"name": "second",
"type": "column",
"data": [
{ "x": 21, "y": 82, "$raw": { "a": 21, "b": 82, "g": "second" } },
{ "x": 95, "y": 32, "$raw": { "a": 95, "b": 32, "g": "second" } }
]
},
{
"name": "first",
"type": "column",
"data": [
{ "x": 42, "y": 10, "$raw": { "a": 42, "b": 10, "g": "first" } },
{ "x": 62, "y": 73, "$raw": { "a": 62, "b": 73, "g": "first" } },
{ "x": 85, "y": 50, "$raw": { "a": 85, "b": 50, "g": "first" } }
]
}
]
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
{
"input": {
"data": [
{ "a": 42, "b": 10, "c": 41, "d": 92 },
{ "a": 62, "b": 73 },
{ "a": 21, "b": null },
{ "a": 85, "b": 50 },
{ "a": 95 }
],
"options": {
"columnMapping": {
"a": "x",
"b": "y"
},
"seriesOptions": {}
}
},
"output": {
"data": [
{
"name": "b",
"type": "column",
"data": [
{ "x": 42, "y": 10, "$raw": { "a": 42, "b": 10, "c": 41, "d": 92 } },
{ "x": 62, "y": 73, "$raw": { "a": 62, "b": 73 } },
{ "x": 21, "y": null, "$raw": { "a": 21, "b": null } },
{ "x": 85, "y": 50, "$raw": { "a": 85, "b": 50 } }
]
}
]
}
}
6 changes: 1 addition & 5 deletions client/app/visualizations/chart/getChartData.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,11 @@ export default function getChartData(data, options) {
let sizeValue = null;
let zValue = null;

forOwn(row, (v, definition) => {
forOwn(row, (value, definition) => {
definition = '' + definition;
const definitionParts = definition.split('::') || definition.split('__');
const name = definitionParts[0];
const type = mappings ? mappings[definition] : definitionParts[1];
let value = v;

if (type === 'unused') {
return;
Expand All @@ -42,9 +41,6 @@ export default function getChartData(data, options) {
point[type] = value;
}
if (type === 'y') {
if (value == null) {
value = 0;
}
yValues[name] = value;
point[type] = value;
}
Expand Down
32 changes: 32 additions & 0 deletions client/app/visualizations/chart/getChartData.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/* eslint-disable global-require, import/no-unresolved */
import getChartData from './getChartData';

describe('Visualizations', () => {
describe('Chart', () => {
describe('getChartData', () => {
test('Single series', () => {
const { input, output } = require('./fixtures/getChartData/single-series');
const data = getChartData(input.data, input.options);
expect(data).toEqual(output.data);
});

test('Multiple series: multiple Y mappings', () => {
const { input, output } = require('./fixtures/getChartData/multiple-series-multiple-y');
const data = getChartData(input.data, input.options);
expect(data).toEqual(output.data);
});

test('Multiple series: grouped', () => {
const { input, output } = require('./fixtures/getChartData/multiple-series-grouped');
const data = getChartData(input.data, input.options);
expect(data).toEqual(output.data);
});

test('Multiple series: sorted', () => {
const { input, output } = require('./fixtures/getChartData/multiple-series-sorted');
const data = getChartData(input.data, input.options);
expect(data).toEqual(output.data);
});
});
});
});
2 changes: 2 additions & 0 deletions client/app/visualizations/chart/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ const DEFAULT_OPTIONS = {
percentFormat: '0[.]00%',
// dateTimeFormat: 'DD/MM/YYYY HH:mm', // will be set from clientConfig
textFormat: '', // default: combination of {{ @@yPercent }} ({{ @@y }} ± {{ @@yError }})

missingValuesAsZero: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure this should be the default? (putting aside the risk of messing up existing charts)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's exactly for not messing up existing charts 🙂 The other option is to update existing charts by migration (which I really don't want to do)

Copy link
Member

Choose a reason for hiding this comment

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

It's the current behavior. If we were to start from the beginning maybe it wouldn't be a default.

};

function initEditorForm(options, columns) {
Expand Down
100 changes: 100 additions & 0 deletions client/app/visualizations/chart/plotly/applyLayoutFixes.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
import { find, pick, reduce } from 'lodash';

function fixLegendContainer(plotlyElement) {
const legend = plotlyElement.querySelector('.legend');
if (legend) {
let node = legend.parentNode;
while (node) {
if (node.tagName.toLowerCase() === 'svg') {
node.style.overflow = 'visible';
break;
}
node = node.parentNode;
}
}
}

export default function applyLayoutFixes(plotlyElement, layout, updatePlot) {
// update layout size to plot container
layout.width = Math.floor(plotlyElement.offsetWidth);
layout.height = Math.floor(plotlyElement.offsetHeight);

const transformName = find([
'transform',
'WebkitTransform',
'MozTransform',
'MsTransform',
'OTransform',
], prop => prop in plotlyElement.style);

if (layout.width <= 600) {
// change legend orientation to horizontal; plotly has a bug with this
// legend alignment - it does not preserve enough space under the plot;
// so we'll hack this: update plot (it will re-render legend), compute
// legend height, reduce plot size by legend height (but not less than
// half of plot container's height - legend will have max height equal to
// plot height), re-render plot again and offset legend to the space under
// the plot.
layout.legend = {
orientation: 'h',
// locate legend inside of plot area - otherwise plotly will preserve
// some amount of space under the plot; also this will limit legend height
// to plot's height
y: 0,
x: 0,
xanchor: 'left',
yanchor: 'bottom',
};

// set `overflow: visible` to svg containing legend because later we will
// position legend outside of it
fixLegendContainer(plotlyElement);

updatePlot(plotlyElement, pick(layout, ['width', 'height', 'legend'])).then(() => {
const legend = plotlyElement.querySelector('.legend'); // eslint-disable-line no-shadow
if (legend) {
// compute real height of legend - items may be split into few columnns,
// also scrollbar may be shown
const bounds = reduce(legend.querySelectorAll('.traces'), (result, node) => {
const b = node.getBoundingClientRect();
result = result || b;
return {
top: Math.min(result.top, b.top),
bottom: Math.max(result.bottom, b.bottom),
};
}, null);
// here we have two values:
// 1. height of plot container excluding height of legend items;
// it may be any value between 0 and plot container's height;
// 2. half of plot containers height. Legend cannot be larger than
// plot; if legend is too large, plotly will reduce it's height and
// show a scrollbar; in this case, height of plot === height of legend,
// so we can split container's height half by half between them.
layout.height = Math.floor(Math.max(
layout.height / 2,
layout.height - (bounds.bottom - bounds.top),
));
// offset the legend
legend.style[transformName] = 'translate(0, ' + layout.height + 'px)';
updatePlot(plotlyElement, pick(layout, ['height']));
}
});
} else {
layout.legend = {
orientation: 'v',
// vertical legend will be rendered properly, so just place it to the right
// side of plot
y: 1,
x: 1,
xanchor: 'left',
yanchor: 'top',
};

const legend = plotlyElement.querySelector('.legend');
if (legend) {
legend.style[transformName] = null;
}

updatePlot(plotlyElement, pick(layout, ['width', 'height', 'legend']));
}
}
Loading