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

fix(chart): set y domain appropriately #61

Merged
merged 1 commit into from
Jul 29, 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
3 changes: 2 additions & 1 deletion .eslintrc
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,12 @@
"ignoreUrls": true
}
],
"no-case-declarations": 0,
"no-console": 0,
"no-debugger": 1,
"no-lonely-if": 1,
"no-plusplus": 0,
"no-case-declarations": 0,
"no-restricted-properties": [0, {"object": "Math", "property": "pow"}],
"prettier/prettier": [
"error",
{
Expand Down
44 changes: 41 additions & 3 deletions src/common/__tests__/__snapshots__/graphHelpers.test.js.snap
Original file line number Diff line number Diff line change
@@ -1,5 +1,38 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`GraphHelpers getChartDomain should return expected y domain for given inputs 1`] = `
Object {
"empty array": Array [
0,
10,
],
"maxY = 0": Array [
0,
10,
],
"maxY = 100": Array [
0,
1000,
],
"maxY = 1000": Array [
0,
10000,
],
"maxY = 49": Array [
0,
50,
],
"maxY = 50": Array [
0,
100,
],
"maxY = 9": Array [
0,
10,
],
}
`;

exports[`GraphHelpers should convert graph data and generate tooltips when usage is populated: usage populated 1`] = `
Object {
"chartData": Array [
Expand Down Expand Up @@ -37,6 +70,10 @@ Object {
0,
31,
],
"y": Array [
0,
10,
],
},
}
`;
Expand Down Expand Up @@ -72,7 +109,7 @@ Object {
],
"y": Array [
0,
100,
10,
],
},
}
Expand Down Expand Up @@ -109,7 +146,7 @@ Object {
],
"y": Array [
0,
100,
10,
],
},
}
Expand Down Expand Up @@ -146,7 +183,7 @@ Object {
],
"y": Array [
0,
100,
10,
],
},
}
Expand All @@ -156,6 +193,7 @@ exports[`GraphHelpers should have specific functions: graphHelpers 1`] = `
Object {
"chartDateFormat": "MMM D",
"convertGraphUsageData": [Function],
"getChartDomain": [Function],
"getGraphHeight": [Function],
"getTooltipDimensions": [Function],
"getTooltipFontSize": [Function],
Expand Down
13 changes: 13 additions & 0 deletions src/common/__tests__/graphHelpers.test.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {
graphHelpers,
convertGraphUsageData,
getChartDomain,
getGraphHeight,
getTooltipDimensions,
getTooltipFontSize
Expand Down Expand Up @@ -94,6 +95,18 @@ describe('GraphHelpers', () => {
).toMatchSnapshot('throws error');
});

it('getChartDomain should return expected y domain for given inputs', () => {
expect({
'empty array': getChartDomain({ empty: true }).y,
'maxY = 0': getChartDomain({ maxY: 0 }).y,
'maxY = 9': getChartDomain({ maxY: 9 }).y,
'maxY = 49': getChartDomain({ maxY: 49 }).y,
'maxY = 50': getChartDomain({ maxY: 50 }).y,
'maxY = 100': getChartDomain({ maxY: 100 }).y,
'maxY = 1000': getChartDomain({ maxY: 1000 }).y
}).toMatchSnapshot();
});

it('should match graph heights at all breakpoints', () => {
const { breakpoints } = helpers;

Expand Down
26 changes: 19 additions & 7 deletions src/common/graphHelpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,18 +93,26 @@ const fillMissingValues = ({ startDate, endDate, values, label, previousLabel })
};

/**
* Returns chart domain setting for given inputs
* Returns chart domain setting for given inputs.
*
* the y axis max should be rounded to the nearest 10 if less than 50,
* otherwise round to the nearest power of 10
*
* the y axis returns large enough number that zeroed bars dont show
*
* @param empty {boolean} Chart data is empty
* @param maxY {boolean} The max y-value
* @returns {Object}
*/
const getChartDomain = ({ empty }) => {
// todo: daily vs weekly
const getChartDomain = ({ empty, maxY = 0 }) => {
const chartDomain = { x: [0, 31] };
if (empty) {
chartDomain.y = [0, 100];

if (empty || maxY < 50) {
chartDomain.y = [0, Math.ceil((maxY + 1) / 10) * 10];
} else {
chartDomain.y = [0, Math.pow(10, maxY.toString().length)];
}

return chartDomain;
};

Expand All @@ -127,9 +135,9 @@ const convertGraphUsageData = ({ data, startDate, endDate, label, previousLabel
let chartDomain = {};

try {
chartDomain = getChartDomain({});

const values = {};
let maxY = 0;

for (let i = 0; i < data.length; i++) {
const formattedDate = moment
.utc(data[i][rhelApiTypes.RHSM_API_RESPONSE_PRODUCTS_DATA_DATE])
Expand All @@ -139,7 +147,9 @@ const convertGraphUsageData = ({ data, startDate, endDate, label, previousLabel
x: formattedDate,
y: data[i][rhelApiTypes.RHSM_API_RESPONSE_PRODUCTS_DATA_SOCKETS]
};
maxY = values[formattedDate].y > maxY ? values[formattedDate].y : maxY;
}
chartDomain = getChartDomain({ maxY });

if (data.length) {
chartData = fillMissingValues({ startDate, endDate, values, label, previousLabel });
Expand Down Expand Up @@ -193,6 +203,7 @@ const getTooltipFontSize = (breakpoints, currentBreakpoint) => {
const graphHelpers = {
chartDateFormat,
convertGraphUsageData,
getChartDomain,
getGraphHeight,
getTooltipDimensions,
getTooltipFontSize,
Expand All @@ -204,6 +215,7 @@ export {
graphHelpers,
chartDateFormat,
convertGraphUsageData,
getChartDomain,
getGraphHeight,
getTooltipDimensions,
getTooltipFontSize,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,10 @@ exports[`RhelGraphCard Component should render multiple states: fulfilled 1`] =
0,
31,
],
"y": Array [
0,
10,
],
}
}
domainPadding={
Expand Down