Chart: Improve render performance (#27606)

This PR refactors the Chart component to be a PureComponent. This prevents unnecessary re-renders when mousing over the chart bars (and triggering a re-render of each `<Bar />` in addition to the new `<Tooltip />`). - Chart: Clean up xAxis - Chart: Clean up barContainer, convert to PureComp - Chart: Modularize bar tooltip into separate comp - Chart: Remove Chart.componentWillReceiveProps - Chart: Convert Label into PureComponent - Chart: Modularize LegendItem, convert to PureComp
Automattic · Oct 9, 2018 · 9eee17f · 9eee17f
1 parent af24346
commit 9eee17f
Show file tree

Hide file tree

Showing 8 changed files with 139 additions and 125 deletions.
diff --git a/client/components/chart/bar-container.jsx b/client/components/chart/bar-container.jsx
@@ -13,42 +13,36 @@ import React from 'react';
 import Bar from './bar';
 import XAxis from './x-axis';
 
-export default class extends React.Component {
-	static displayName = 'ModuleChartBarContainer';
-
+export default class ChartBarContainer extends React.PureComponent {
 	static propTypes = {
-		isTouch: PropTypes.bool,
-		data: PropTypes.array,
-		yAxisMax: PropTypes.number,
-		width: PropTypes.number,
 		barClick: PropTypes.func,
+		data: PropTypes.array,
 		isRtl: PropTypes.bool,
-	};
-
-	buildBars = max => {
-		return this.props.data.map( function( item, index ) {
-			return (
-				<Bar
-					index={ index }
-					key={ index }
-					isTouch={ this.props.isTouch }
-					className={ item.className }
-					clickHandler={ this.props.barClick }
-					data={ item }
-					max={ max }
-					count={ this.props.data.length }
-					chartWidth={ this.props.chartWidth }
-					setTooltip={ this.props.setTooltip }
-					isRtl={ this.props.isRtl }
-				/>
-			);
-		}, this );
+		isTouch: PropTypes.bool,
+		width: PropTypes.number,
+		yAxisMax: PropTypes.number,
 	};
 
 	render() {
 		return (
 			<div>
-				<div className="chart__bars">{ this.buildBars( this.props.yAxisMax ) }</div>
+				<div className="chart__bars">
+					{ this.props.data.map( ( item, index ) => (
+						<Bar
+							index={ index }
+							key={ index }
+							isTouch={ this.props.isTouch }
+							className={ item.className }
+							clickHandler={ this.props.barClick }
+							data={ item }
+							max={ this.props.yAxisMax }
+							count={ this.props.data.length }
+							chartWidth={ this.props.chartWidth }
+							setTooltip={ this.props.setTooltip }
+							isRtl={ this.props.isRtl }
+						/>
+					) ) }
+				</div>
 				<XAxis data={ this.props.data } labelWidth={ 42 } isRtl={ this.props.isRtl } />
 			</div>
 		);

diff --git a/client/components/chart/bar-tooltip.jsx b/client/components/chart/bar-tooltip.jsx
@@ -0,0 +1,32 @@
+/** @format */
+
+/**
+ * External dependencies
+ */
+import PropTypes from 'prop-types';
+import React from 'react';
+import classNames from 'classnames';
+import Gridicon from 'gridicons';
+
+export default class ChartBarTooltip extends React.PureComponent {
+	static propTypes = {
+		className: PropTypes.string,
+		icon: PropTypes.string,
+		label: PropTypes.string,
+		value: PropTypes.string,
+	};
+
+	render() {
+		return (
+			<li className={ classNames( 'module-content-list-item', this.props.className ) }>
+				<span className="chart__tooltip-wrapper wrapper">
+					<span className="chart__tooltip-value value">{ this.props.value }</span>
+					<span className="chart__tooltip-label label">
+						{ this.props.icon && <Gridicon icon={ this.props.icon } size={ 18 } /> }
+						{ this.props.label }
+					</span>
+				</span>
+			</li>
+		);
+	}
+}
diff --git a/client/components/chart/bar.jsx b/client/components/chart/bar.jsx
@@ -4,20 +4,24 @@
  * External dependencies
  */
 import PropTypes from 'prop-types';
-import React, { Component } from 'react';
+import React from 'react';
 import classNames from 'classnames';
-import Gridicon from 'gridicons';
 
-export default class ModuleChartBar extends Component {
+/**
+ * Internal dependencies
+ */
+import ChartBarTooltip from './bar-tooltip';
+
+export default class ChartBar extends React.PureComponent {
 	static propTypes = {
-		isTouch: PropTypes.bool,
-		tooltipPosition: PropTypes.string,
 		className: PropTypes.string,
 		clickHandler: PropTypes.func,
-		data: PropTypes.object.isRequired,
-		max: PropTypes.number,
 		count: PropTypes.number,
+		data: PropTypes.object.isRequired,
 		isRtl: PropTypes.bool,
+		isTouch: PropTypes.bool,
+		max: PropTypes.number,
+		tooltipPosition: PropTypes.string,
 	};
 
 	static defaultProps = {
@@ -64,29 +68,13 @@ export default class ModuleChartBar extends Component {
 	};
 
 	getTooltipData() {
-		const { tooltipData } = this.props.data;
-
-		return tooltipData.map( function( options, i ) {
-			return (
-				<li key={ i } className={ classNames( 'module-content-list-item', options.className ) }>
-					<span className="chart__tooltip-wrapper wrapper">
-						<span className="chart__tooltip-value value">{ options.value }</span>
-						<span className="chart__tooltip-label label">
-							{ options.icon && <Gridicon icon={ options.icon } size={ 18 } /> }
-							{ options.label }
-						</span>
-					</span>
-				</li>
-			);
+		return this.props.data.tooltipData.map( function( options, i ) {
+			return <ChartBarTooltip key={ i } { ...options } />;
 		} );
 	}
 
 	getPercentage() {
-		const {
-			data: { value },
-			max,
-		} = this.props;
-		return Math.ceil( ( value / max ) * 10000 ) / 100;
+		return Math.ceil( ( this.props.data.value / this.props.max ) * 10000 ) / 100;
 	}
 
 	getNestedPercentage() {

diff --git a/client/components/chart/index.jsx b/client/components/chart/index.jsx
@@ -2,7 +2,7 @@
 /**
  * External dependencies
  */
-import * as React from 'react';
+import React from 'react';
 import PropTypes from 'prop-types';
 import { localize } from 'i18n-calypso';
 import { noop } from 'lodash';
@@ -34,48 +34,46 @@ class Chart extends React.Component {
 	};
 
 	static propTypes = {
-		loading: PropTypes.bool,
+		barClick: PropTypes.func,
 		data: PropTypes.array,
-		minTouchBarWidth: PropTypes.number,
+		isRtl: PropTypes.bool,
+		loading: PropTypes.bool,
 		minBarWidth: PropTypes.number,
-		barClick: PropTypes.func,
-		translate: PropTypes.func,
+		minTouchBarWidth: PropTypes.number,
 		numberFormat: PropTypes.func,
-		isRtl: PropTypes.bool,
+		translate: PropTypes.func,
 	};
 
 	static defaultProps = {
-		minTouchBarWidth: 42,
-		minBarWidth: 15,
 		barClick: noop,
+		minBarWidth: 15,
+		minTouchBarWidth: 42,
 	};
 
 	componentDidMount() {
 		this.resize = afterLayoutFlush( this.resize );
 		window.addEventListener( 'resize', this.resize );
 
-		const { data, loading } = this.props;
-
-		if ( data && data.length && ! loading ) {
-			this.resize( this.props );
+		if ( this.props.data && this.props.data.length && ! this.props.loading ) {
+			this.resize();
 		}
 	}
 
-	componentWillUnmount() {
-		window.removeEventListener( 'resize', this.resize );
-	}
-
-	componentWillReceiveProps( nextProps ) {
-		if ( this.props.loading && ! nextProps.loading ) {
-			return this.resize( nextProps );
+	componentDidUpdate( prevProps ) {
+		if ( prevProps.loading && ! this.props.loading ) {
+			return this.resize();
 		}
 
-		if ( nextProps.data !== this.props.data ) {
-			this.updateData( nextProps );
+		if ( prevProps.data !== this.props.data ) {
+			this.updateData( this.props );
 		}
 	}
 
-	resize = props => {
+	componentWillUnmount() {
+		window.removeEventListener( 'resize', this.resize );
+	}
+
+	resize = ( props = this.props ) => {
 		if ( ! this.chart ) {
 			return;
 		}
@@ -166,12 +164,12 @@ class Chart extends React.Component {
 				</div>
 				<BarContainer
 					barClick={ barClick }
+					chartWidth={ width }
 					data={ data }
-					yAxisMax={ yMax }
+					isRtl={ this.props.isRtl }
 					isTouch={ hasTouch() }
-					chartWidth={ width }
 					setTooltip={ this.setTooltip }
-					isRtl={ this.props.isRtl }
+					yAxisMax={ yMax }
 				/>
 				{ isTooltipVisible && (
 					<Tooltip

diff --git a/client/components/chart/label.jsx b/client/components/chart/label.jsx
@@ -3,23 +3,20 @@
 /**
  * External dependencies
  */
-
 import PropTypes from 'prop-types';
 import React from 'react';
 
-export default class ModuleChartLabel extends React.Component {
+export default class ChartLabel extends React.PureComponent {
 	static propTypes = {
 		isRtl: PropTypes.bool,
+		label: PropTypes.string.isRequired,
 		width: PropTypes.number.isRequired,
 		x: PropTypes.number.isRequired,
-		label: PropTypes.string.isRequired,
 	};
 
 	render() {
-		const { isRtl } = this.props;
-
 		const labelStyle = {
-			[ isRtl ? 'right' : 'left' ]: this.props.x + 'px',
+			[ this.props.isRtl ? 'right' : 'left' ]: this.props.x + 'px',
 			width: this.props.width + 'px',
 		};
 

diff --git a/client/components/chart/legend-item.jsx b/client/components/chart/legend-item.jsx
@@ -0,0 +1,37 @@
+/** @format */
+
+/**
+ * External dependencies
+ */
+import PropTypes from 'prop-types';
+import React from 'react';
+
+export default class ChartLegendItem extends React.PureComponent {
+	static propTypes = {
+		attr: PropTypes.string.isRequired,
+		changeHandler: PropTypes.func.isRequired,
+		checked: PropTypes.bool.isRequired,
+		label: PropTypes.oneOfType( [ PropTypes.object, PropTypes.string ] ),
+	};
+
+	clickHandler = () => {
+		this.props.changeHandler( this.props.attr );
+	};
+
+	render() {
+		return (
+			<li className="chart__legend-option">
+				<label className="chart__legend-label is-selectable">
+					<input
+						checked={ this.props.checked }
+						className="chart__legend-checkbox"
+						onChange={ this.clickHandler }
+						type="checkbox"
+					/>
+					<span className={ this.props.className } />
+					{ this.props.label }
+				</label>
+			</li>
+		);
+	}
+}
diff --git a/client/components/chart/legend.jsx b/client/components/chart/legend.jsx
@@ -3,46 +3,16 @@
 /**
  * External dependencies
  */
-
 import PropTypes from 'prop-types';
-import React, { PureComponent, Component } from 'react';
+import React from 'react';
 import { find, noop } from 'lodash';
 
 /**
- * Module variables
+ * Internal dependencies
  */
+import ChartLegendItem from './legend-item';
 
-class LegendItem extends PureComponent {
-	static propTypes = {
-		attr: PropTypes.string.isRequired,
-		changeHandler: PropTypes.func.isRequired,
-		checked: PropTypes.bool.isRequired,
-		label: PropTypes.oneOfType( [ PropTypes.object, PropTypes.string ] ),
-	};
-
-	clickHandler = () => {
-		this.props.changeHandler( this.props.attr );
-	};
-
-	render() {
-		return (
-			<li className="chart__legend-option">
-				<label className="chart__legend-label is-selectable">
-					<input
-						checked={ this.props.checked }
-						className="chart__legend-checkbox"
-						onChange={ this.clickHandler }
-						type="checkbox"
-					/>
-					<span className={ this.props.className } />
-					{ this.props.label }
-				</label>
-			</li>
-		);
-	}
-}
-
-class Legend extends Component {
+export default class ChartLegend extends React.PureComponent {
 	static propTypes = {
 		activeCharts: PropTypes.array,
 		activeTab: PropTypes.object.isRequired,
@@ -72,7 +42,7 @@ class Legend extends Component {
 				tab = find( this.props.tabs, { attr: legendItem } );
 
 			return (
-				<LegendItem
+				<ChartLegendItem
 					key={ index }
 					className={ colorClass }
 					label={ tab.label }
@@ -98,5 +68,3 @@ class Legend extends Component {
 		);
 	}
 }
-
-export default Legend;