From 7f4bb8df630132caa67f64f2ecc72263603dc4cd Mon Sep 17 00:00:00 2001 From: Ron Rennick Date: Thu, 14 Feb 2019 13:35:08 -0400 Subject: [PATCH 1/6] update chart type query string key to chartType --- .../components/report-chart/index.js | 2 +- .../dashboard/dashboard-charts/index.js | 21 +++++++------- .../components/src/chart/d3chart/chart.js | 28 +++++++++---------- .../src/chart/d3chart/d3base/index.js | 4 +-- .../src/chart/d3chart/utils/axis.js | 4 +-- .../packages/components/src/chart/index.js | 27 +++++++++--------- .../packages/date/src/index.js | 6 ++-- 7 files changed, 47 insertions(+), 45 deletions(-) diff --git a/plugins/woocommerce-admin/client/analytics/components/report-chart/index.js b/plugins/woocommerce-admin/client/analytics/components/report-chart/index.js index f356fee7248..b73ee63e21a 100644 --- a/plugins/woocommerce-admin/client/analytics/components/report-chart/index.js +++ b/plugins/woocommerce-admin/client/analytics/components/report-chart/index.js @@ -125,7 +125,7 @@ export class ReportChart extends Component { tooltipLabelFormat={ formats.tooltipLabelFormat } tooltipTitle={ ( 'time-comparison' === mode && selectedChart.label ) || null } tooltipValueFormat={ getTooltipValueFormat( selectedChart.type ) } - type={ getChartTypeForQuery( query ) } + chartType={ getChartTypeForQuery( query ) } valueType={ selectedChart.type } xFormat={ formats.xFormat } x2Format={ formats.x2Format } diff --git a/plugins/woocommerce-admin/client/dashboard/dashboard-charts/index.js b/plugins/woocommerce-admin/client/dashboard/dashboard-charts/index.js index 5de7c54e8f0..5547c82f582 100644 --- a/plugins/woocommerce-admin/client/dashboard/dashboard-charts/index.js +++ b/plugins/woocommerce-admin/client/dashboard/dashboard-charts/index.js @@ -78,11 +78,11 @@ class DashboardCharts extends Component { }; } - handleTypeToggle( type ) { + handleTypeToggle( chartType ) { return () => { - this.setState( { chartType: type } ); + this.setState( { chartType: chartType } ); const userDataFields = { - [ 'dashboard_chart_type' ]: type, + [ 'dashboard_chart_type' ]: chartType, }; this.props.updateCurrentUserData( userDataFields ); }; @@ -146,7 +146,7 @@ class DashboardCharts extends Component { render() { const { path } = this.props; const { chartType, hiddenChartKeys, interval } = this.state; - const query = { ...this.props.query, type: chartType, interval }; + const query = { ...this.props.query, chartType: chartType, interval }; return (
@@ -163,24 +163,25 @@ class DashboardCharts extends Component { > } title={ __( 'Line chart', 'wc-admin' ) } - aria-checked={ query.type === 'line' } + aria-checked={ query.chartType === 'line' } role="menuitemradio" - tabIndex={ query.type === 'line' ? 0 : -1 } + tabIndex={ query.chartType === 'line' ? 0 : -1 } onClick={ this.handleTypeToggle( 'line' ) } /> } title={ __( 'Bar chart', 'wc-admin' ) } - aria-checked={ query.type === 'bar' } + aria-checked={ query.chartType === 'bar' } role="menuitemradio" - tabIndex={ query.type === 'bar' ? 0 : -1 } + tabIndex={ query.chartType === 'bar' ? 0 : -1 } onClick={ this.handleTypeToggle( 'bar' ) } /> 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 c00fdc5ba11..53eb6a14936 100644 --- a/plugins/woocommerce-admin/packages/components/src/chart/d3chart/chart.js +++ b/plugins/woocommerce-admin/packages/components/src/chart/d3chart/chart.js @@ -46,7 +46,7 @@ class D3Chart extends Component { } drawChart( node ) { - const { data, margin, type } = this.props; + const { data, margin, chartType } = this.props; const params = this.getParams(); const adjParams = Object.assign( {}, params, { height: params.adjHeight, @@ -60,18 +60,18 @@ class D3Chart extends Component { .append( 'g' ) .attr( 'transform', `translate(${ margin.left },${ margin.top })` ); - const xOffset = type === 'line' && adjParams.uniqueDates.length <= 1 + const xOffset = chartType === 'line' && adjParams.uniqueDates.length <= 1 ? adjParams.width / 2 : 0; drawAxis( g, adjParams, xOffset ); - type === 'line' && drawLines( g, data, adjParams, xOffset ); - type === 'bar' && drawBars( g, data, adjParams ); + chartType === 'line' && drawLines( g, data, adjParams, xOffset ); + chartType === 'bar' && drawBars( g, data, adjParams ); } shouldBeCompact() { - const { data, margin, type, width } = this.props; - if ( type !== 'bar' ) { + const { data, margin, chartType, width } = this.props; + if ( chartType !== 'bar' ) { return false; } const widthWithoutMargins = width - margin.left - margin.right; @@ -82,8 +82,8 @@ class D3Chart extends Component { } getWidth() { - const { data, margin, type, width } = this.props; - if ( type !== 'bar' ) { + const { data, margin, chartType, width } = this.props; + if ( chartType !== 'bar' ) { return width; } const columnsPerDate = data && data.length ? Object.keys( data[ 0 ] ).length - 1 : 0; @@ -106,7 +106,7 @@ class D3Chart extends Component { tooltipLabelFormat, tooltipValueFormat, tooltipTitle, - type, + chartType, xFormat, x2Format, yFormat, @@ -144,7 +144,7 @@ class D3Chart extends Component { tooltipLabelFormat: getFormatter( tooltipLabelFormat, d3TimeFormat ), tooltipValueFormat: getFormatter( tooltipValueFormat ), tooltipTitle, - type, + chartType, uniqueDates, uniqueKeys, valueType, @@ -172,7 +172,7 @@ class D3Chart extends Component { } render() { - const { className, data, height, type } = this.props; + const { className, data, height, chartType } = this.props; const computedWidth = this.getWidth(); return (
@@ -268,7 +268,7 @@ D3Chart.propTypes = { /** * Chart type of either `line` or `bar`. */ - type: PropTypes.oneOf( [ 'bar', 'line' ] ), + chartType: PropTypes.oneOf( [ 'bar', 'line' ] ), /** * Width of the `svg`. */ @@ -302,7 +302,7 @@ D3Chart.defaultProps = { tooltipPosition: 'over', tooltipLabelFormat: '%B %d, %Y', tooltipValueFormat: ',', - type: 'line', + chartType: 'line', width: 600, xFormat: '%Y-%m-%d', x2Format: '', diff --git a/plugins/woocommerce-admin/packages/components/src/chart/d3chart/d3base/index.js b/plugins/woocommerce-admin/packages/components/src/chart/d3chart/d3base/index.js index 5140f8ac8ac..78abe142c42 100644 --- a/plugins/woocommerce-admin/packages/components/src/chart/d3chart/d3base/index.js +++ b/plugins/woocommerce-admin/packages/components/src/chart/d3chart/d3base/index.js @@ -47,7 +47,7 @@ export default class D3Base extends Component { ! isEqual( this.props.orderedKeys, nextProps.orderedKeys ) || this.props.drawChart !== nextProps.drawChart || this.props.height !== nextProps.height || - this.props.type !== nextProps.type || + this.props.chartType !== nextProps.chartType || this.props.width !== nextProps.width ); } @@ -105,5 +105,5 @@ D3Base.propTypes = { className: PropTypes.string, data: PropTypes.array, orderedKeys: PropTypes.array, // required to detect changes in data - type: PropTypes.string, + chartType: PropTypes.string, }; 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 7846a85b574..13939e97849 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 @@ -192,7 +192,7 @@ export const getYGrids = ( yMax ) => { }; export const drawAxis = ( node, params, xOffset ) => { - const xScale = params.type === 'line' ? params.xLineScale : params.xScale; + const xScale = params.chartType === 'line' ? params.xLineScale : params.xScale; const removeDuplicateDates = ( d, i, ticks, formatter ) => { const monthDate = moment( d ).toDate(); let prevMonth = i !== 0 ? ticks[ i - 1 ] : ticks[ i ]; @@ -204,7 +204,7 @@ export const drawAxis = ( node, params, xOffset ) => { const yGrids = getYGrids( params.yMax === 0 ? 1 : params.yMax ); - const ticks = params.xTicks.map( d => ( params.type === 'line' ? moment( d ).toDate() : d ) ); + const ticks = params.xTicks.map( d => ( params.chartType === 'line' ? moment( d ).toDate() : d ) ); node .append( 'g' ) diff --git a/plugins/woocommerce-admin/packages/components/src/chart/index.js b/plugins/woocommerce-admin/packages/components/src/chart/index.js index 9291b30b9a9..57d43efede2 100644 --- a/plugins/woocommerce-admin/packages/components/src/chart/index.js +++ b/plugins/woocommerce-admin/packages/components/src/chart/index.js @@ -143,10 +143,10 @@ class Chart extends Component { window.removeEventListener( 'resize', this.updateDimensions ); } - handleTypeToggle( type ) { - if ( this.props.type !== type ) { + handleTypeToggle( chartType ) { + if ( this.props.chartType !== chartType ) { const { path, query } = this.props; - updateQueryString( { type }, path, query ); + updateQueryString( { chartType }, path, query ); } } @@ -279,7 +279,7 @@ class Chart extends Component { tooltipLabelFormat, tooltipValueFormat, tooltipTitle, - type, + chartType, valueType, xFormat, x2Format, @@ -289,6 +289,7 @@ class Chart extends Component { const legendPosition = this.getLegendPosition(); const legendDirection = legendPosition === 'top' ? 'row' : 'column'; const chartDirection = legendPosition === 'side' ? 'row' : 'column'; +// const type = chartType; const chartHeight = this.getChartHeight(); const legend = isRequesting ? null : ( @@ -335,24 +336,24 @@ class Chart extends Component { > } title={ __( 'Line chart', 'wc-admin' ) } - aria-checked={ type === 'line' } + aria-checked={ chartType === 'line' } role="menuitemradio" - tabIndex={ type === 'line' ? 0 : -1 } + tabIndex={ chartType === 'line' ? 0 : -1 } onClick={ partial( this.handleTypeToggle, 'line' ) } /> } title={ __( 'Bar chart', 'wc-admin' ) } - aria-checked={ type === 'bar' } + aria-checked={ chartType === 'bar' } role="menuitemradio" - tabIndex={ type === 'bar' ? 0 : -1 } + tabIndex={ chartType === 'bar' ? 0 : -1 } onClick={ partial( this.handleTypeToggle, 'bar' ) } /> @@ -392,7 +393,7 @@ class Chart extends Component { tooltipValueFormat={ tooltipValueFormat } tooltipPosition={ isViewportLarge ? 'over' : 'below' } tooltipTitle={ tooltipTitle } - type={ type } + chartType={ chartType } width={ chartDirection === 'row' ? width - 320 : width } xFormat={ xFormat } x2Format={ x2Format } @@ -494,7 +495,7 @@ Chart.propTypes = { /** * Chart type of either `line` or `bar`. */ - type: PropTypes.oneOf( [ 'bar', 'line' ] ), + chartType: PropTypes.oneOf( [ 'bar', 'line' ] ), /** * What type of data is to be displayed? Number, Average, String? */ @@ -524,7 +525,7 @@ Chart.defaultProps = { showHeaderControls: true, tooltipLabelFormat: '%B %d, %Y', tooltipValueFormat: ',', - type: 'line', + chartType: 'line', xFormat: '%d', x2Format: '%b %Y', yFormat: '$.3s', diff --git a/plugins/woocommerce-admin/packages/date/src/index.js b/plugins/woocommerce-admin/packages/date/src/index.js index 4dc6455a481..3f739b4601f 100644 --- a/plugins/woocommerce-admin/packages/date/src/index.js +++ b/plugins/woocommerce-admin/packages/date/src/index.js @@ -425,9 +425,9 @@ export function getIntervalForQuery( query ) { * @param {Object} query Current query * @return {String} Current chart type. */ -export function getChartTypeForQuery( { type } ) { - if ( [ 'line', 'bar' ].includes( type ) ) { - return type; +export function getChartTypeForQuery( { chartType } ) { + if ( [ 'line', 'bar' ].includes( chartType ) ) { + return chartType; } return 'line'; } From 1422865efd63b4ea9a9eee0fbdc5404b402dd017 Mon Sep 17 00:00:00 2001 From: Ron Rennick Date: Thu, 14 Feb 2019 13:40:25 -0400 Subject: [PATCH 2/6] remove stray debugging code --- plugins/woocommerce-admin/packages/components/src/chart/index.js | 1 - 1 file changed, 1 deletion(-) diff --git a/plugins/woocommerce-admin/packages/components/src/chart/index.js b/plugins/woocommerce-admin/packages/components/src/chart/index.js index 57d43efede2..021d7eec356 100644 --- a/plugins/woocommerce-admin/packages/components/src/chart/index.js +++ b/plugins/woocommerce-admin/packages/components/src/chart/index.js @@ -289,7 +289,6 @@ class Chart extends Component { const legendPosition = this.getLegendPosition(); const legendDirection = legendPosition === 'top' ? 'row' : 'column'; const chartDirection = legendPosition === 'side' ? 'row' : 'column'; -// const type = chartType; const chartHeight = this.getChartHeight(); const legend = isRequesting ? null : ( From 41251c3e9c3effb88054dd534acaa3b78b3836fe Mon Sep 17 00:00:00 2001 From: Ron Rennick Date: Thu, 14 Feb 2019 15:31:20 -0400 Subject: [PATCH 3/6] fix scss whitespace --- .../packages/components/src/chart/d3chart/style.scss | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 b295155f7d1..1f37a9019d8 100644 --- a/plugins/woocommerce-admin/packages/components/src/chart/d3chart/style.scss +++ b/plugins/woocommerce-admin/packages/components/src/chart/d3chart/style.scss @@ -158,12 +158,12 @@ .focus-grid { line { - stroke: rgba( 0, 0, 0, 0.1 ); + stroke: rgba(0, 0, 0, 0.1); stroke-width: 1px; } } .barfocus { - fill: rgba( 0, 0, 0, 0.1 ); + fill: rgba(0, 0, 0, 0.1); } } From e7c55d7ca589e4b649e57e2b25d75e3c09bb7675 Mon Sep 17 00:00:00 2001 From: Ron Rennick Date: Thu, 14 Feb 2019 15:34:57 -0400 Subject: [PATCH 4/6] update query string for chart type tests --- plugins/woocommerce-admin/packages/date/test/index.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/woocommerce-admin/packages/date/test/index.js b/plugins/woocommerce-admin/packages/date/test/index.js index 78c8914ed68..0d306650a36 100644 --- a/plugins/woocommerce-admin/packages/date/test/index.js +++ b/plugins/woocommerce-admin/packages/date/test/index.js @@ -697,7 +697,7 @@ describe( 'getPreviousDate', () => { describe( 'getChartTypeForQuery', () => { it( 'should return allowed type', () => { const query = { - type: 'bar', + chartType: 'bar', }; expect( getChartTypeForQuery( query ) ).toBe( 'bar' ); } ); @@ -708,7 +708,7 @@ describe( 'getChartTypeForQuery', () => { it( 'should return line for not allowed type', () => { const query = { - type: 'burrito', + chartType: 'burrito', }; expect( getChartTypeForQuery( query ) ).toBe( 'line' ); } ); From 86c89e741a32f03fb127ee2ed97af4a4a95401d9 Mon Sep 17 00:00:00 2001 From: Ron Rennick Date: Thu, 14 Feb 2019 16:03:54 -0400 Subject: [PATCH 5/6] one more type -> chartType --- .../packages/components/src/chart/d3chart/utils/axis.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 6d684b4c40c..5cb18b4ece1 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 @@ -198,7 +198,7 @@ const removeDuplicateDates = ( d, i, ticks, formatter ) => { const drawXAxis = ( node, params, scales, formats ) => { const height = scales.yScale.range()[ 0 ]; let ticks = getXTicks( params.uniqueDates, scales.xScale.range()[ 1 ], params.mode, params.interval ); - if ( params.type === 'line' ) { + if ( params.chartType === 'line' ) { ticks = ticks.map( d => moment( d ).toDate() ); } From 9f911df05300cc8f53dfd8419baecdfb2236e63d Mon Sep 17 00:00:00 2001 From: Ron Rennick Date: Fri, 15 Feb 2019 10:37:56 -0400 Subject: [PATCH 6/6] update changelog, coding style per review --- .../client/dashboard/dashboard-charts/index.js | 4 ++-- .../woocommerce-admin/packages/components/CHANGELOG.md | 1 + .../packages/components/src/chart/index.js | 10 +++++----- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/plugins/woocommerce-admin/client/dashboard/dashboard-charts/index.js b/plugins/woocommerce-admin/client/dashboard/dashboard-charts/index.js index 5547c82f582..6055fe7eff4 100644 --- a/plugins/woocommerce-admin/client/dashboard/dashboard-charts/index.js +++ b/plugins/woocommerce-admin/client/dashboard/dashboard-charts/index.js @@ -80,7 +80,7 @@ class DashboardCharts extends Component { handleTypeToggle( chartType ) { return () => { - this.setState( { chartType: chartType } ); + this.setState( { chartType } ); const userDataFields = { [ 'dashboard_chart_type' ]: chartType, }; @@ -146,7 +146,7 @@ class DashboardCharts extends Component { render() { const { path } = this.props; const { chartType, hiddenChartKeys, interval } = this.state; - const query = { ...this.props.query, chartType: chartType, interval }; + const query = { ...this.props.query, chartType, interval }; return (
diff --git a/plugins/woocommerce-admin/packages/components/CHANGELOG.md b/plugins/woocommerce-admin/packages/components/CHANGELOG.md index 9baff23e2bf..5025416dcb2 100644 --- a/plugins/woocommerce-admin/packages/components/CHANGELOG.md +++ b/plugins/woocommerce-admin/packages/components/CHANGELOG.md @@ -2,6 +2,7 @@ - Chart component: new props `emptyMessage` and `baseValue`. When an empty message is provided, it will be displayed on top of the chart if there are no values different than `baseValue`. - Chart component: remove d3-array dependency. - Chart component: fix display when there is no data. +- Chart component: change chart type query parameter to `chartType`. - Improves display of charts where all values are 0. - Fix X-axis labels in hourly bar charts. - New `` prop named `showClearButton`, that will display a 'Clear' button when the search box contains one or more tags. diff --git a/plugins/woocommerce-admin/packages/components/src/chart/index.js b/plugins/woocommerce-admin/packages/components/src/chart/index.js index 021d7eec356..42e3cd3a484 100644 --- a/plugins/woocommerce-admin/packages/components/src/chart/index.js +++ b/plugins/woocommerce-admin/packages/components/src/chart/index.js @@ -420,6 +420,10 @@ Chart.propTypes = { * `emptyMessage` will be displayed if provided. */ baseValue: PropTypes.number, + /** + * Chart type of either `line` or `bar`. + */ + chartType: PropTypes.oneOf( [ 'bar', 'line' ] ), /** * An array of data. */ @@ -491,10 +495,6 @@ Chart.propTypes = { * A string to use as a title for the tooltip. Takes preference over `tooltipLabelFormat`. */ tooltipTitle: PropTypes.string, - /** - * Chart type of either `line` or `bar`. - */ - chartType: PropTypes.oneOf( [ 'bar', 'line' ] ), /** * What type of data is to be displayed? Number, Average, String? */ @@ -515,6 +515,7 @@ Chart.propTypes = { Chart.defaultProps = { baseValue: 0, + chartType: 'line', data: [], dateParser: '%Y-%m-%dT%H:%M:%S', interactiveLegend: true, @@ -524,7 +525,6 @@ Chart.defaultProps = { showHeaderControls: true, tooltipLabelFormat: '%B %d, %Y', tooltipValueFormat: ',', - chartType: 'line', xFormat: '%d', x2Format: '%b %Y', yFormat: '$.3s',