From 7d8df3ecfcbad6ce6493e2410ec36af7a18b4ce3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Albert=20Juh=C3=A9=20Lluveras?= Date: Wed, 3 Apr 2019 18:19:38 +0200 Subject: [PATCH] Allow negative values in charts (https://github.com/woocommerce/woocommerce-admin/pull/1979) * Add support for negative charts * Always display main axis at 0 * Improve tests * Make sure positive and negative grid steps are the same * Code style improvements * Fix missing lines when all values are 0 * Make sure empty note always appear on top * Add CHANGELOG entry --- .../packages/components/CHANGELOG.md | 1 + .../components/src/chart/d3chart/chart.js | 10 +++- .../components/src/chart/d3chart/style.scss | 11 ++-- .../src/chart/d3chart/utils/axis.js | 48 ++++++++++++---- .../src/chart/d3chart/utils/bar-chart.js | 5 +- .../src/chart/d3chart/utils/scales.js | 55 ++++++++++++++----- .../src/chart/d3chart/utils/test/axis.js | 54 +++++++++++++++--- .../src/chart/d3chart/utils/test/scales.js | 27 +++++---- 8 files changed, 159 insertions(+), 52 deletions(-) diff --git a/plugins/woocommerce-admin/packages/components/CHANGELOG.md b/plugins/woocommerce-admin/packages/components/CHANGELOG.md index eb8e4789279..78053dd544b 100644 --- a/plugins/woocommerce-admin/packages/components/CHANGELOG.md +++ b/plugins/woocommerce-admin/packages/components/CHANGELOG.md @@ -1,5 +1,6 @@ # (unreleased) - Chart legend component now uses withInstanceId HOC so the ids used in several HTML elements are unique. +- Chart component now accepts data with negative values. - Chart component: new prop `filterParam` used to detect selected items in the current query. If there are, they will be displayed in the chart even if their values are 0. - Expand search results and allow searching when input is refocused in autocompleter. diff --git a/plugins/woocommerce-admin/packages/components/src/chart/d3chart/chart.js b/plugins/woocommerce-admin/packages/components/src/chart/d3chart/chart.js index d15654419d2..bf163c83408 100644 --- a/plugins/woocommerce-admin/packages/components/src/chart/d3chart/chart.js +++ b/plugins/woocommerce-admin/packages/components/src/chart/d3chart/chart.js @@ -22,8 +22,8 @@ import { getXScale, getXGroupScale, getXLineScale, - getYMax, getYScale, + getYScaleLimits, } from './utils/scales'; import { drawAxis } from './utils/axis'; import { drawBars } from './utils/bar-chart'; @@ -62,13 +62,15 @@ class D3Chart extends Component { const adjHeight = height - margin.top - margin.bottom; const adjWidth = this.getWidth() - margin.left - margin.right; - const yMax = getYMax( data ); - const yScale = getYScale( adjHeight, yMax ); + const { upper: yMax, lower: yMin, step } = getYScaleLimits( data ); + const yScale = getYScale( adjHeight, yMin, yMax ); if ( chartType === 'line' ) { return { + step, xScale: getXLineScale( uniqueDates, adjWidth ), yMax, + yMin, yScale, }; } @@ -77,9 +79,11 @@ class D3Chart extends Component { const xScale = getXScale( uniqueDates, adjWidth, compact ); return { + step, xGroupScale: getXGroupScale( orderedKeys, xScale, compact ), xScale, yMax, + yMin, yScale, }; } diff --git a/plugins/woocommerce-admin/packages/components/src/chart/d3chart/style.scss b/plugins/woocommerce-admin/packages/components/src/chart/d3chart/style.scss index 1f37a9019d8..3b0fd0b2dd9 100644 --- a/plugins/woocommerce-admin/packages/components/src/chart/d3chart/style.scss +++ b/plugins/woocommerce-admin/packages/components/src/chart/d3chart/style.scss @@ -29,10 +29,12 @@ margin: 0 auto; max-width: 50%; padding-bottom: 48px; + pointer-events: none; position: absolute; right: 0; top: 0; text-align: center; + z-index: 1; @include breakpoint( '<782px' ) { @include font-size( 13 ); @@ -129,14 +131,11 @@ stroke: $core-grey-dark-500; } } - - &:last-child { - line { - opacity: 0; - } - } } } + .grid.with-positive-ticks .tick:last-child line { + opacity: 0; + } .tick { padding-top: 10px; stroke-width: 1; diff --git a/plugins/woocommerce-admin/packages/components/src/chart/d3chart/utils/axis.js b/plugins/woocommerce-admin/packages/components/src/chart/d3chart/utils/axis.js index a39dc2ef90a..a76506246fe 100644 --- a/plugins/woocommerce-admin/packages/components/src/chart/d3chart/utils/axis.js +++ b/plugins/woocommerce-admin/packages/components/src/chart/d3chart/utils/axis.js @@ -173,17 +173,44 @@ export const compareStrings = ( s1, s2, splitChar = new RegExp( [ ' |,' ], 'g' ) return diff; }; -export const getYGrids = ( yMax ) => { - const yGrids = []; +const calculateYGridValues = ( numberOfTicks, limit, roundValues ) => { + const grids = []; - for ( let i = 0; i < 4; i++ ) { - const value = yMax > 1 ? Math.round( i / 3 * yMax ) : i / 3 * yMax; - if ( yGrids[ yGrids.length - 1 ] !== value ) { - yGrids.push( value ); + for ( let i = 0; i < numberOfTicks; i++ ) { + const val = ( i + 1 ) / numberOfTicks * limit; + const rVal = roundValues ? Math.round( val ) : val; + if ( grids[ grids.length - 1 ] !== rVal ) { + grids.push( rVal ); } } - return yGrids; + return grids; +}; + +const getNegativeYGrids = ( yMin, step ) => { + if ( yMin >= 0 ) { + return []; + } + + const numberOfTicks = Math.ceil( -yMin / step ); + return calculateYGridValues( numberOfTicks, yMin, yMin < -1 ); +}; + +const getPositiveYGrids = ( yMax, step ) => { + if ( yMax <= 0 ) { + return []; + } + + const numberOfTicks = Math.ceil( yMax / step ); + return calculateYGridValues( numberOfTicks, yMax, yMax > 1 ); +}; + +export const getYGrids = ( yMin, yMax, step ) => { + return [ + 0, + ...getNegativeYGrids( yMin, step ), + ...getPositiveYGrids( yMax, step ), + ]; }; const removeDuplicateDates = ( d, i, ticks, formatter ) => { @@ -239,13 +266,14 @@ const drawXAxis = ( node, params, scales, formats ) => { }; const drawYAxis = ( node, scales, formats, margin, isRTL ) => { - const yGrids = getYGrids( scales.yScale.domain()[ 1 ] ); + const yGrids = getYGrids( scales.yScale.domain()[ 0 ], scales.yScale.domain()[ 1 ], scales.step ); const width = scales.xScale.range()[ 1 ]; const xPosition = isRTL ? width + margin.left + margin.right / 2 - 15 : -margin.left / 2 - 15; + const withPositiveValuesClass = scales.yMin >= 0 || scales.yMax > 0 ? ' with-positive-ticks' : ''; node .append( 'g' ) - .attr( 'class', 'grid' ) + .attr( 'class', 'grid' + withPositiveValuesClass ) .attr( 'transform', `translate(-${ margin.left }, 0)` ) .call( d3AxisLeft( scales.yScale ) @@ -262,7 +290,7 @@ const drawYAxis = ( node, scales, formats, margin, isRTL ) => { .attr( 'text-anchor', 'start' ) .call( d3AxisLeft( scales.yScale ) - .tickValues( scales.yMax === 0 ? [ yGrids[ 0 ] ] : yGrids ) + .tickValues( scales.yMax === 0 && scales.yMin === 0 ? [ yGrids[ 0 ] ] : yGrids ) .tickFormat( d => formats.yFormat( d !== 0 ? d : 0 ) ) ); }; diff --git a/plugins/woocommerce-admin/packages/components/src/chart/d3chart/utils/bar-chart.js b/plugins/woocommerce-admin/packages/components/src/chart/d3chart/utils/bar-chart.js index 60f029ff317..35bedb2e598 100644 --- a/plugins/woocommerce-admin/packages/components/src/chart/d3chart/utils/bar-chart.js +++ b/plugins/woocommerce-admin/packages/components/src/chart/d3chart/utils/bar-chart.js @@ -40,6 +40,7 @@ export const drawBars = ( node, data, params, scales, formats, tooltip ) => { } ) .on( 'mouseout', () => tooltip.hide() ); + const basePosition = scales.yScale( 0 ); barGroup .selectAll( '.bar' ) .data( d => @@ -56,9 +57,9 @@ export const drawBars = ( node, data, params, scales, formats, tooltip ) => { .append( 'rect' ) .attr( 'class', 'bar' ) .attr( 'x', d => scales.xGroupScale( d.key ) ) - .attr( 'y', d => scales.yScale( d.value ) ) + .attr( 'y', d => Math.min( basePosition, scales.yScale( d.value ) ) ) .attr( 'width', scales.xGroupScale.bandwidth() ) - .attr( 'height', d => height - scales.yScale( d.value ) ) + .attr( 'height', d => Math.abs( basePosition - scales.yScale( d.value ) ) ) .attr( 'fill', d => params.getColor( d.key ) ) .attr( 'pointer-events', 'none' ) .attr( 'tabindex', '0' ) diff --git a/plugins/woocommerce-admin/packages/components/src/chart/d3chart/utils/scales.js b/plugins/woocommerce-admin/packages/components/src/chart/d3chart/utils/scales.js index 26f2adb17a9..e24b6ecb2e9 100644 --- a/plugins/woocommerce-admin/packages/components/src/chart/d3chart/utils/scales.js +++ b/plugins/woocommerce-admin/packages/components/src/chart/d3chart/utils/scales.js @@ -52,41 +52,70 @@ export const getXLineScale = ( uniqueDates, width ) => ] ) .rangeRound( [ 0, width ] ); -const getMaxYValue = data => { +const getYValueLimits = data => { let maxYValue = Number.NEGATIVE_INFINITY; + let minYValue = Number.POSITIVE_INFINITY; data.map( d => { for ( const [ key, item ] of Object.entries( d ) ) { if ( key !== 'date' && Number.isFinite( item.value ) && item.value > maxYValue ) { maxYValue = item.value; } + if ( key !== 'date' && Number.isFinite( item.value ) && item.value < minYValue ) { + minYValue = item.value; + } } } ); - return maxYValue; + return { upper: maxYValue, lower: minYValue }; +}; + +const calculateStep = ( minValue, maxValue ) => { + if ( ! Number.isFinite( minValue ) || ! Number.isFinite( maxValue ) ) { + return 1; + } + + const maxAbsValue = Math.max( -minValue, maxValue ); + const maxLimit = 4 / 3 * maxAbsValue; + const pow3Y = Math.pow( 10, ( ( Math.log( maxLimit ) * Math.LOG10E + 1 ) | 0 ) - 2 ) * 3; + + return Math.max( Math.ceil( Math.ceil( maxLimit / pow3Y ) * pow3Y / 3 ), 1 / 3 ); }; /** - * Describes and rounds the maximum y value to the nearest thousand, ten-thousand, million etc. In case it is a decimal number, ceils it. + * Returns the lower and upper limits of the Y scale and the calculated step to use in the axis, rounding + * them to the nearest thousand, ten-thousand, million etc. In case it is a decimal number, ceils it. * @param {array} data - The chart component's `data` prop. - * @returns {number} the maximum value in the timeseries multiplied by 4/3 + * @returns {object} Object containing the `lower` and `upper` limits and a `step` value. */ -export const getYMax = data => { - const maxValue = getMaxYValue( data ); - if ( ! Number.isFinite( maxValue ) || maxValue <= 0 ) { - return 0; +export const getYScaleLimits = data => { + const { lower: minValue, upper: maxValue } = getYValueLimits( data ); + const step = calculateStep( minValue, maxValue ); + const limits = { lower: 0, upper: 0, step }; + + if ( Number.isFinite( minValue ) || minValue < 0 ) { + limits.lower = Math.floor( minValue / step ) * step; + if ( limits.lower === minValue && minValue !== 0 ) { + limits.lower -= step; + } } - const yMax = 4 / 3 * maxValue; - const pow3Y = Math.pow( 10, ( ( Math.log( yMax ) * Math.LOG10E + 1 ) | 0 ) - 2 ) * 3; - return Math.ceil( Math.ceil( yMax / pow3Y ) * pow3Y ); + if ( Number.isFinite( maxValue ) || maxValue > 0 ) { + limits.upper = Math.ceil( maxValue / step ) * step; + if ( limits.upper === maxValue && maxValue !== 0 ) { + limits.upper += step; + } + } + + return limits; }; /** * Describes getYScale * @param {number} height - calculated height of the charting space + * @param {number} yMin - minimum y value * @param {number} yMax - maximum y value * @returns {function} the D3 linear scale from 0 to the value from `getYMax` */ -export const getYScale = ( height, yMax ) => +export const getYScale = ( height, yMin, yMax ) => d3ScaleLinear() - .domain( [ 0, yMax === 0 ? 1 : yMax ] ) + .domain( [ Math.min( yMin, 0 ), yMax === 0 && yMin === 0 ? 1 : Math.max( yMax, 0 ) ] ) .rangeRound( [ height, 0 ] ); diff --git a/plugins/woocommerce-admin/packages/components/src/chart/d3chart/utils/test/axis.js b/plugins/woocommerce-admin/packages/components/src/chart/d3chart/utils/test/axis.js index e2900410660..4518ed5cb78 100644 --- a/plugins/woocommerce-admin/packages/components/src/chart/d3chart/utils/test/axis.js +++ b/plugins/woocommerce-admin/packages/components/src/chart/d3chart/utils/test/axis.js @@ -240,19 +240,57 @@ describe( 'compareStrings', () => { } ); describe( 'getYGrids', () => { - it( 'returns a single 0 when yMax is 0', () => { - expect( getYGrids( 0 ) ).toEqual( [ 0 ] ); + it( 'returns a single 0 when yMax and yMin are 0', () => { + expect( getYGrids( 0, 0, 0 ) ).toEqual( [ 0 ] ); } ); - it( 'returns decimal values when yMax is <= 1', () => { - expect( getYGrids( 1 ) ).toEqual( [ 0, 0.3333333333333333, 0.6666666666666666, 1 ] ); + describe( 'positive charts', () => { + it( 'returns decimal values when yMax is <= 1 and yMin is 0', () => { + expect( getYGrids( 0, 1, 0.3333333333333333 ) ).toEqual( [ 0, 0.3333333333333333, 0.6666666666666666, 1 ] ); + } ); + + it( 'returns decimal values when yMax and yMin are <= 1', () => { + expect( getYGrids( 1, 1, 0.3333333333333333 ) ).toEqual( [ 0, 0.3333333333333333, 0.6666666666666666, 1 ] ); + } ); + + it( 'doesn\'t return decimal values when yMax is > 1', () => { + expect( getYGrids( 0, 2, 1 ) ).toEqual( [ 0, 1, 2 ] ); + } ); + + it( 'returns up to four values when yMax is a big number', () => { + expect( getYGrids( 0, 12000, 4000 ) ).toEqual( [ 0, 4000, 8000, 12000 ] ); + } ); } ); - it( 'doesn\'t return decimal values when yMax is >1', () => { - expect( getYGrids( 2 ) ).toEqual( [ 0, 1, 2 ] ); + describe( 'negative charts', () => { + it( 'returns decimal values when yMin is >= -1 and yMax is 0', () => { + expect( getYGrids( -1, 0, 0.3333333333333333 ) ).toEqual( [ 0, -0.3333333333333333, -0.6666666666666666, -1 ] ); + } ); + + it( 'returns decimal values when yMax and yMin are >= -1', () => { + expect( getYGrids( -1, -1, 0.3333333333333333 ) ).toEqual( [ 0, -0.3333333333333333, -0.6666666666666666, -1 ] ); + } ); + + it( 'doesn\'t return decimal values when yMin is < -1', () => { + expect( getYGrids( -2, 0, 1 ) ).toEqual( [ 0, -1, -2 ] ); + } ); + + it( 'returns up to four values when yMin is a big negative number', () => { + expect( getYGrids( -12000, 0, 4000 ) ).toEqual( [ 0, -4000, -8000, -12000 ] ); + } ); } ); - it( 'returns up to four values when yMax is a big number', () => { - expect( getYGrids( 10000 ) ).toEqual( [ 0, 3333, 6667, 10000 ] ); + describe( 'positive & negative charts', () => { + it( 'returns decimal values when yMax is <= 1 and yMin is 0', () => { + expect( getYGrids( -1, 1, 0.5 ) ).toEqual( [ 0, -0.5, -1, 0.5, 1 ] ); + } ); + + it( 'doesn\'t return decimal values when yMax is > 1', () => { + expect( getYGrids( -2, 2, 1 ) ).toEqual( [ 0, -1, -2, 1, 2 ] ); + } ); + + it( 'returns up to six values when yMax is a big number', () => { + expect( getYGrids( -12000, 12000, 6000 ) ).toEqual( [ 0, -6000, -12000, 6000, 12000 ] ); + } ); } ); } ); diff --git a/plugins/woocommerce-admin/packages/components/src/chart/d3chart/utils/test/scales.js b/plugins/woocommerce-admin/packages/components/src/chart/d3chart/utils/test/scales.js index bc591fee68b..7588abec489 100644 --- a/plugins/woocommerce-admin/packages/components/src/chart/d3chart/utils/test/scales.js +++ b/plugins/woocommerce-admin/packages/components/src/chart/d3chart/utils/test/scales.js @@ -12,7 +12,7 @@ import { getOrderedKeys, getUniqueDates, } from '../index'; -import { getXGroupScale, getXScale, getXLineScale, getYMax, getYScale } from '../scales'; +import { getXGroupScale, getXScale, getXLineScale, getYScaleLimits, getYScale } from '../scales'; jest.mock( 'd3-scale', () => ( { ...require.requireActual( 'd3-scale' ), @@ -88,26 +88,33 @@ describe( 'X scales', () => { } ); describe( 'Y scales', () => { - describe( 'getYMax', () => { - it( 'calculate the correct maximum y value', () => { - expect( getYMax( dummyOrders ) ).toEqual( 15000000 ); + describe( 'getYScaleLimits', () => { + it( 'calculate the correct y value limits', () => { + expect( getYScaleLimits( dummyOrders ) ).toEqual( { lower: 0, upper: 15000000, step: 5000000 } ); } ); - it( 'return 0 if there is no line data', () => { - expect( getYMax( [] ) ).toEqual( 0 ); + it( 'return defaults if there is no line data', () => { + expect( getYScaleLimits( [] ) ).toEqual( { lower: 0, upper: 0, step: 1 } ); } ); } ); describe( 'getYScale', () => { - it( 'creates linear scale with correct parameters', () => { - getYScale( 100, 15000000 ); + it( 'creates positive linear scale with correct parameters', () => { + getYScale( 100, 0, 15000000 ); expect( scaleLinear().domain ).toHaveBeenLastCalledWith( [ 0, 15000000 ] ); expect( scaleLinear().rangeRound ).toHaveBeenLastCalledWith( [ 100, 0 ] ); } ); - it( 'avoids the domain starting and ending at the same point when yMax is 0', () => { - getYScale( 100, 0 ); + it( 'creates negative linear scale with correct parameters', () => { + getYScale( 100, -15000000, 0 ); + + expect( scaleLinear().domain ).toHaveBeenLastCalledWith( [ -15000000, 0 ] ); + expect( scaleLinear().rangeRound ).toHaveBeenLastCalledWith( [ 100, 0 ] ); + } ); + + it( 'avoids the domain starting and ending at the same point when yMin, yMax are 0', () => { + getYScale( 100, 0, 0 ); const args = scaleLinear().domain.mock.calls; const lastArgs = args[ args.length - 1 ][ 0 ];