Skip to content

Commit

Permalink
Fix: Prevent some needless re-rendering in high-level components (#31958
Browse files Browse the repository at this point in the history
) (#32357)

In an effort to remove the workpad renaming lag in #25070, I started hunting down and fixing re-renders that happened with the name change. 

I was hoping this PR would fix the lag, but I've shifted to some other performance issues that have a much larger impact, so this is just a bunch of re-rendering fixes, which come with some performance gains. Here's the breakdown:

- `Workpad`, `WorkpadHeader`, and `FullscreenControl` would always re-render because the hotkey handler function was always re-created
- `Workpad` would re-render when the workpad's name changed
  - We were passing the whole workpad object into the component, so it re-rendered on all sorts of changes
  - Page title updating moved to middleware so the component doesn't need that value
- `AssetManager` would always re-render if the parent re-rendered because it always created a new state value in `mapStateToProps`
- make `Workpad` and `Toolbar` pure, they take no props and this helps stop some re-rendering
  • Loading branch information
w33ble authored Mar 5, 2019
1 parent e735314 commit 6030ac4
Show file tree
Hide file tree
Showing 10 changed files with 285 additions and 246 deletions.
1 change: 1 addition & 0 deletions x-pack/plugins/canvas/common/lib/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,4 @@ export const FETCH_TIMEOUT = 30000; // 30 seconds
export const CANVAS_USAGE_TYPE = 'canvas';
export const DEFAULT_WORKPAD_CSS = '.canvasPage {\n\n}';
export const VALID_IMAGE_TYPES = ['gif', 'jpeg', 'png', 'svg+xml'];
export const ASSET_MAX_SIZE = 25000;
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,11 @@ import { ConfirmModal } from '../confirm_modal';
import { Clipboard } from '../clipboard';
import { Download } from '../download';
import { Loading } from '../loading';
import { ASSET_MAX_SIZE } from '../../../common/lib/constants';

export class AssetManager extends React.PureComponent {
static propTypes = {
assets: PropTypes.array,
assetValues: PropTypes.array,
addImageElement: PropTypes.func,
removeAsset: PropTypes.func.isRequired,
copyAsset: PropTypes.func.isRequired,
Expand Down Expand Up @@ -147,15 +148,13 @@ export class AssetManager extends React.PureComponent {

render() {
const { isModalVisible, loading } = this.state;
const { assets } = this.props;

const assetMaxLimit = 25000;
const { assetValues } = this.props;

const assetsTotal = Math.round(
assets.reduce((total, asset) => total + asset.value.length, 0) / 1024
assetValues.reduce((total, { value }) => total + value.length, 0) / 1024
);

const percentageUsed = Math.round((assetsTotal / assetMaxLimit) * 100);
const percentageUsed = Math.round((assetsTotal / ASSET_MAX_SIZE) * 100);

const emptyAssets = (
<EuiPanel className="canvasAssetManager__emptyPanel">
Expand Down Expand Up @@ -208,9 +207,9 @@ export class AssetManager extends React.PureComponent {
</p>
</EuiText>
<EuiSpacer />
{assets.length ? (
{assetValues.length ? (
<EuiFlexGrid responsive={false} columns={4}>
{assets.map(this.renderAsset)}
{assetValues.map(this.renderAsset)}
</EuiFlexGrid>
) : (
emptyAssets
Expand All @@ -221,7 +220,7 @@ export class AssetManager extends React.PureComponent {
<EuiFlexItem>
<EuiProgress
value={assetsTotal}
max={assetMaxLimit}
max={ASSET_MAX_SIZE}
color={percentageUsed < 90 ? 'secondary' : 'danger'}
size="s"
aria-labelledby="CanvasAssetManagerLabel"
Expand Down
11 changes: 7 additions & 4 deletions x-pack/plugins/canvas/public/components/asset_manager/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import { VALID_IMAGE_TYPES } from '../../../common/lib/constants';
import { AssetManager as Component } from './asset_manager';

const mapStateToProps = state => ({
assets: Object.values(getAssets(state)), // pull values out of assets object
assets: getAssets(state),
selectedPage: getSelectedPage(state),
});

Expand Down Expand Up @@ -60,19 +60,22 @@ const mapDispatchToProps = dispatch => ({
});

const mergeProps = (stateProps, dispatchProps, ownProps) => {
const { assets } = stateProps;
const { assets, selectedPage } = stateProps;
const { onAssetAdd } = dispatchProps;
const assetValues = Object.values(assets); // pull values out of assets object

return {
...ownProps,
...stateProps,
...dispatchProps,
selectedPage,
assetValues,
addImageElement: dispatchProps.addImageElement(stateProps.selectedPage),
onAssetAdd: file => {
const [type, subtype] = get(file, 'type', '').split('/');
if (type === 'image' && VALID_IMAGE_TYPES.indexOf(subtype) >= 0) {
return encode(file).then(dataurl => {
const type = 'dataurl';
const existingId = findExistingAsset(type, dataurl, assets);
const existingId = findExistingAsset(type, dataurl, assetValues);
if (existingId) {
return existingId;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,15 @@ import PropTypes from 'prop-types';
import { Shortcuts } from 'react-shortcuts';

export class FullscreenControl extends React.PureComponent {
keyHandler = action => {
const enterFullscreen = action === 'FULLSCREEN';
const exitFullscreen = this.props.isFullscreen && action === 'FULLSCREEN_EXIT';

if (enterFullscreen || exitFullscreen) {
this.toggleFullscreen();
}
};

toggleFullscreen = () => {
const { setFullscreen, isFullscreen } = this.props;
setFullscreen(!isFullscreen);
Expand All @@ -17,17 +26,11 @@ export class FullscreenControl extends React.PureComponent {
render() {
const { children, isFullscreen } = this.props;

const keyHandler = action => {
if (action === 'FULLSCREEN' || (isFullscreen && action === 'FULLSCREEN_EXIT')) {
this.toggleFullscreen();
}
};

return (
<span>
<Shortcuts
name="PRESENTATION"
handler={keyHandler}
handler={this.keyHandler}
targetNodeSelector="body"
global
isolate
Expand Down
3 changes: 2 additions & 1 deletion x-pack/plugins/canvas/public/components/toolbar/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

import { connect } from 'react-redux';
import PropTypes from 'prop-types';
import { compose, withState, getContext, withHandlers } from 'recompose';
import { pure, compose, withState, getContext, withHandlers } from 'recompose';

import {
getWorkpad,
Expand All @@ -26,6 +26,7 @@ const mapStateToProps = state => ({
});

export const Toolbar = compose(
pure,
connect(mapStateToProps),
getContext({
router: PropTypes.object,
Expand Down
29 changes: 18 additions & 11 deletions x-pack/plugins/canvas/public/components/workpad/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

import { connect } from 'react-redux';
import PropTypes from 'prop-types';
import { compose, withState, withProps, getContext, withHandlers } from 'recompose';
import { pure, compose, withState, withProps, getContext, withHandlers } from 'recompose';
import { transitionsRegistry } from '../../lib/transitions_registry';
import { undoHistory, redoHistory } from '../../state/actions/history';
import { fetchAllRenderables } from '../../state/actions/elements';
Expand All @@ -19,13 +19,19 @@ import {
} from '../../state/selectors/workpad';
import { Workpad as Component } from './workpad';

const mapStateToProps = state => ({
pages: getPages(state),
selectedPageNumber: getSelectedPageIndex(state) + 1,
totalElementCount: getAllElements(state).length,
workpad: getWorkpad(state),
isFullscreen: getFullscreen(state),
});
const mapStateToProps = state => {
const { width, height, id: workpadId, css: workpadCss } = getWorkpad(state);
return {
pages: getPages(state),
selectedPageNumber: getSelectedPageIndex(state) + 1,
totalElementCount: getAllElements(state).length,
width,
height,
workpadCss,
workpadId,
isFullscreen: getFullscreen(state),
};
};

const mapDispatchToProps = {
undoHistory,
Expand All @@ -34,6 +40,7 @@ const mapDispatchToProps = {
};

export const Workpad = compose(
pure,
getContext({
router: PropTypes.object,
}),
Expand Down Expand Up @@ -68,17 +75,17 @@ export const Workpad = compose(
}
props.setPrevSelectedPageNumber(props.selectedPageNumber);
const transitionPage = Math.max(props.selectedPageNumber, pageNumber) - 1;
const { transition } = props.workpad.pages[transitionPage];
const { transition } = props.pages[transitionPage];
if (transition) {
props.setTransition(transition);
}
props.router.navigateTo('loadWorkpad', { id: props.workpad.id, page: pageNumber });
props.router.navigateTo('loadWorkpad', { id: props.workpadId, page: pageNumber });
},
}),
withHandlers({
onTransitionEnd: ({ setTransition }) => () => setTransition(null),
nextPage: props => () => {
const pageNumber = Math.min(props.selectedPageNumber + 1, props.workpad.pages.length);
const pageNumber = Math.min(props.selectedPageNumber + 1, props.pages.length);
props.onPageChange(pageNumber);
},
previousPage: props => () => {
Expand Down
Loading

0 comments on commit 6030ac4

Please sign in to comment.