Fix: Prevent some needless re-rendering in high-level components (#31958) (#32359)

In an effort to remove the workpad renaming lag in https://github.com/elastic/kibana/issues/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
This commit is contained in:
Joe Fleming 2019-03-05 10:10:30 -07:00 committed by GitHub
parent 62d2123048
commit 72c7595742
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
10 changed files with 285 additions and 246 deletions

View file

@ -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;

View file

@ -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,
@ -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">
@ -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
@ -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"

View file

@ -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),
});
@ -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;
}

View file

@ -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);
@ -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

View file

@ -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,
@ -26,6 +26,7 @@ const mapStateToProps = state => ({
});
export const Toolbar = compose(
pure,
connect(mapStateToProps),
getContext({
router: PropTypes.object,

View file

@ -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';
@ -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,
@ -34,6 +40,7 @@ const mapDispatchToProps = {
};
export const Workpad = compose(
pure,
getContext({
router: PropTypes.object,
}),
@ -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 => () => {

View file

@ -10,33 +10,41 @@ import { Shortcuts } from 'react-shortcuts';
import Style from 'style-it';
import { WorkpadPage } from '../workpad_page';
import { Fullscreen } from '../fullscreen';
import { setDocTitle } from '../../lib/doc_title';
export const Workpad = props => {
const {
selectedPageNumber,
getAnimation,
onTransitionEnd,
pages,
totalElementCount,
workpad,
fetchAllRenderables,
undoHistory,
redoHistory,
setGrid, // TODO: Get rid of grid when we improve the layout engine
grid,
nextPage,
previousPage,
isFullscreen,
} = props;
const WORKPAD_CANVAS_BUFFER = 32; // 32px padding around the workpad
const { height, width } = workpad;
const bufferStyle = {
height: isFullscreen ? height : height + 32,
width: isFullscreen ? width : width + 32,
export class Workpad extends React.PureComponent {
static propTypes = {
selectedPageNumber: PropTypes.number.isRequired,
getAnimation: PropTypes.func.isRequired,
onTransitionEnd: PropTypes.func.isRequired,
grid: PropTypes.bool.isRequired,
setGrid: PropTypes.func.isRequired,
pages: PropTypes.array.isRequired,
totalElementCount: PropTypes.number.isRequired,
isFullscreen: PropTypes.bool.isRequired,
width: PropTypes.number.isRequired,
height: PropTypes.number.isRequired,
workpadCss: PropTypes.string,
undoHistory: PropTypes.func.isRequired,
redoHistory: PropTypes.func.isRequired,
nextPage: PropTypes.func.isRequired,
previousPage: PropTypes.func.isRequired,
fetchAllRenderables: PropTypes.func.isRequired,
css: PropTypes.object,
};
const keyHandler = action => {
keyHandler = action => {
const {
fetchAllRenderables,
undoHistory,
redoHistory,
nextPage,
previousPage,
grid, // TODO: Get rid of grid when we improve the layout engine
setGrid,
} = this.props;
// handle keypress events for editor and presentation events
// this exists in both contexts
if (action === 'REFRESH') {
@ -63,84 +71,84 @@ export const Workpad = props => {
}
};
setDocTitle(workpad.name);
render() {
const {
selectedPageNumber,
getAnimation,
onTransitionEnd,
pages,
totalElementCount,
width,
height,
workpadCss,
grid,
isFullscreen,
} = this.props;
return (
<div className="canvasWorkpad__buffer" style={bufferStyle}>
<div className="canvasCheckered" style={{ height, width }}>
{!isFullscreen && (
<Shortcuts name="EDITOR" handler={keyHandler} targetNodeSelector="body" global />
)}
const bufferStyle = {
height: isFullscreen ? height : height + WORKPAD_CANVAS_BUFFER,
width: isFullscreen ? width : width + WORKPAD_CANVAS_BUFFER,
};
<Fullscreen>
{({ isFullscreen, windowSize }) => {
const scale = Math.min(windowSize.height / height, windowSize.width / width);
const fsStyle = isFullscreen
? {
transform: `scale3d(${scale}, ${scale}, 1)`,
WebkitTransform: `scale3d(${scale}, ${scale}, 1)`,
msTransform: `scale3d(${scale}, ${scale}, 1)`,
// height,
// width,
height: windowSize.height < height ? 'auto' : height,
width: windowSize.width < width ? 'auto' : width,
}
: {};
return (
<div className="canvasWorkpad__buffer" style={bufferStyle}>
<div className="canvasCheckered" style={{ height, width }}>
{!isFullscreen && (
<Shortcuts name="EDITOR" handler={this.keyHandler} targetNodeSelector="body" global />
)}
// NOTE: the data-shared-* attributes here are used for reporting
return Style.it(
workpad.css,
<div
className={`canvasWorkpad ${isFullscreen ? 'fullscreen' : ''}`}
style={fsStyle}
data-shared-items-count={totalElementCount}
>
{isFullscreen && (
<Shortcuts
name="PRESENTATION"
handler={keyHandler}
targetNodeSelector="body"
global
/>
)}
{pages.map((page, i) => (
<WorkpadPage
key={page.id}
page={page}
height={height}
width={width}
isSelected={i + 1 === selectedPageNumber}
animation={getAnimation(i + 1)}
onAnimationEnd={onTransitionEnd}
/>
))}
<Fullscreen>
{({ isFullscreen, windowSize }) => {
const scale = Math.min(windowSize.height / height, windowSize.width / width);
const fsStyle = isFullscreen
? {
transform: `scale3d(${scale}, ${scale}, 1)`,
WebkitTransform: `scale3d(${scale}, ${scale}, 1)`,
msTransform: `scale3d(${scale}, ${scale}, 1)`,
// height,
// width,
height: windowSize.height < height ? 'auto' : height,
width: windowSize.width < width ? 'auto' : width,
}
: {};
// NOTE: the data-shared-* attributes here are used for reporting
return Style.it(
workpadCss,
<div
className="canvasGrid"
style={{ height, width, display: grid ? 'block' : 'none' }}
/>
</div>
);
}}
</Fullscreen>
className={`canvasWorkpad ${isFullscreen ? 'fullscreen' : ''}`}
style={fsStyle}
data-shared-items-count={totalElementCount}
>
{isFullscreen && (
<Shortcuts
name="PRESENTATION"
handler={this.keyHandler}
targetNodeSelector="body"
global
/>
)}
{pages.map((page, i) => (
<WorkpadPage
key={page.id}
page={page}
height={height}
width={width}
isSelected={i + 1 === selectedPageNumber}
animation={getAnimation(i + 1)}
onAnimationEnd={onTransitionEnd}
/>
))}
<div
className="canvasGrid"
style={{ height, width, display: grid ? 'block' : 'none' }}
/>
</div>
);
}}
</Fullscreen>
</div>
</div>
</div>
);
};
Workpad.propTypes = {
selectedPageNumber: PropTypes.number.isRequired,
getAnimation: PropTypes.func.isRequired,
onTransitionEnd: PropTypes.func.isRequired,
grid: PropTypes.bool.isRequired,
setGrid: PropTypes.func.isRequired,
pages: PropTypes.array.isRequired,
totalElementCount: PropTypes.number.isRequired,
isFullscreen: PropTypes.bool.isRequired,
workpad: PropTypes.object.isRequired,
undoHistory: PropTypes.func.isRequired,
redoHistory: PropTypes.func.isRequired,
nextPage: PropTypes.func.isRequired,
previousPage: PropTypes.func.isRequired,
fetchAllRenderables: PropTypes.func.isRequired,
css: PropTypes.object,
};
);
}
}

View file

@ -7,7 +7,7 @@
import { compose, withState } from 'recompose';
import { connect } from 'react-redux';
import { canUserWrite } from '../../state/selectors/app';
import { getWorkpadName, getSelectedPage, isWriteable } from '../../state/selectors/workpad';
import { getSelectedPage, isWriteable } from '../../state/selectors/workpad';
import { setWriteable } from '../../state/actions/workpad';
import { addElement } from '../../state/actions/elements';
import { WorkpadHeader as Component } from './workpad_header';
@ -15,7 +15,6 @@ import { WorkpadHeader as Component } from './workpad_header';
const mapStateToProps = state => ({
isWriteable: isWriteable(state) && canUserWrite(state),
canUserWrite: canUserWrite(state),
workpadName: getWorkpadName(state),
selectedPage: getSelectedPage(state),
});
@ -33,10 +32,10 @@ const mergeProps = (stateProps, dispatchProps, ownProps) => ({
});
export const WorkpadHeader = compose(
withState('showElementModal', 'setShowElementModal', false),
connect(
mapStateToProps,
mapDispatchToProps,
mergeProps
)
),
withState('showElementModal', 'setShowElementModal', false)
)(Component);

View file

@ -23,122 +23,134 @@ import { WorkpadExport } from '../workpad_export';
import { FullscreenControl } from '../fullscreen_control';
import { RefreshControl } from '../refresh_control';
export const WorkpadHeader = ({
isWriteable,
canUserWrite,
toggleWriteable,
addElement,
setShowElementModal,
showElementModal,
}) => {
const keyHandler = action => {
export class WorkpadHeader extends React.PureComponent {
static propTypes = {
isWriteable: PropTypes.bool,
toggleWriteable: PropTypes.func,
addElement: PropTypes.func.isRequired,
showElementModal: PropTypes.bool,
setShowElementModal: PropTypes.func,
};
fullscreenButton = ({ toggleFullscreen }) => (
<EuiToolTip position="bottom" content="Enter fullscreen mode">
<EuiButtonIcon
iconType="fullScreen"
aria-label="View fullscreen"
onClick={toggleFullscreen}
/>
</EuiToolTip>
);
keyHandler = action => {
if (action === 'EDITING') {
toggleWriteable();
this.props.toggleWriteable();
}
};
const elementAdd = (
<EuiOverlayMask>
<EuiModal
onClose={() => setShowElementModal(false)}
className="canvasModal--fixedSize"
maxWidth="1000px"
initialFocus=".canvasElements__filter"
>
<ElementTypes
onClick={element => {
addElement(element);
setShowElementModal(false);
}}
/>
<EuiModalFooter>
<EuiButton size="s" onClick={() => setShowElementModal(false)}>
Dismiss
</EuiButton>
</EuiModalFooter>
</EuiModal>
</EuiOverlayMask>
);
elementAdd = () => {
const { addElement, setShowElementModal } = this.props;
let readOnlyToolTip = '';
return (
<EuiOverlayMask>
<EuiModal
onClose={() => setShowElementModal(false)}
className="canvasModal--fixedSize"
maxWidth="1000px"
initialFocus=".canvasElements__filter"
>
<ElementTypes
onClick={element => {
addElement(element);
setShowElementModal(false);
}}
/>
<EuiModalFooter>
<EuiButton size="s" onClick={() => setShowElementModal(false)}>
Dismiss
</EuiButton>
</EuiModalFooter>
</EuiModal>
</EuiOverlayMask>
);
};
if (!canUserWrite) {
readOnlyToolTip = "You don't have permission to edit this workpad";
} else {
readOnlyToolTip = isWriteable ? 'Hide editing controls' : 'Show editing controls';
}
getTooltipText = () => {
if (!this.props.canUserWrite) {
return "You don't have permission to edit this workpad";
} else {
return this.props.isWriteable ? 'Hide editing controls' : 'Show editing controls';
}
};
return (
<div>
{showElementModal ? elementAdd : null}
<EuiFlexGroup gutterSize="s" alignItems="center" justifyContent="spaceBetween">
<EuiFlexItem grow={false}>
<EuiFlexGroup alignItems="center" gutterSize="xs">
<EuiFlexItem grow={false}>
<RefreshControl />
</EuiFlexItem>
<EuiFlexItem grow={false}>
<FullscreenControl>
{({ toggleFullscreen }) => (
<EuiToolTip position="bottom" content="Enter fullscreen mode">
<EuiButtonIcon
iconType="fullScreen"
aria-label="View fullscreen"
onClick={toggleFullscreen}
/>
</EuiToolTip>
)}
</FullscreenControl>
</EuiFlexItem>
<EuiFlexItem grow={false}>
<WorkpadExport />
</EuiFlexItem>
<EuiFlexItem grow={false}>
{canUserWrite && (
<Shortcuts name="EDITOR" handler={keyHandler} targetNodeSelector="body" global />
)}
<EuiToolTip position="bottom" content={readOnlyToolTip}>
<EuiButtonIcon
iconType={isWriteable ? 'lockOpen' : 'lock'}
onClick={() => {
toggleWriteable();
}}
size="s"
aria-label={readOnlyToolTip}
isDisabled={!canUserWrite}
/>
</EuiToolTip>
</EuiFlexItem>
</EuiFlexGroup>
</EuiFlexItem>
{isWriteable ? (
render() {
const {
isWriteable,
canUserWrite,
toggleWriteable,
setShowElementModal,
showElementModal,
} = this.props;
return (
<div>
{showElementModal ? this.elementAdd() : null}
<EuiFlexGroup gutterSize="s" alignItems="center" justifyContent="spaceBetween">
<EuiFlexItem grow={false}>
<EuiFlexGroup alignItems="center" gutterSize="s">
<EuiFlexGroup alignItems="center" gutterSize="xs">
<EuiFlexItem grow={false}>
<AssetManager />
<RefreshControl />
</EuiFlexItem>
<EuiFlexItem grow={false}>
<EuiButton
fill
size="s"
iconType="vector"
onClick={() => setShowElementModal(true)}
>
Add element
</EuiButton>
<FullscreenControl>{this.fullscreenButton}</FullscreenControl>
</EuiFlexItem>
<EuiFlexItem grow={false}>
<WorkpadExport />
</EuiFlexItem>
<EuiFlexItem grow={false}>
{canUserWrite && (
<Shortcuts
name="EDITOR"
handler={this.keyHandler}
targetNodeSelector="body"
global
/>
)}
<EuiToolTip position="bottom" content={this.getTooltipText()}>
<EuiButtonIcon
iconType={isWriteable ? 'lockOpen' : 'lock'}
onClick={() => {
toggleWriteable();
}}
size="s"
aria-label={this.getTooltipText()}
isDisabled={!canUserWrite}
/>
</EuiToolTip>
</EuiFlexItem>
</EuiFlexGroup>
</EuiFlexItem>
) : null}
</EuiFlexGroup>
</div>
);
};
WorkpadHeader.propTypes = {
isWriteable: PropTypes.bool,
toggleWriteable: PropTypes.func,
addElement: PropTypes.func.isRequired,
showElementModal: PropTypes.bool,
setShowElementModal: PropTypes.func,
};
{isWriteable ? (
<EuiFlexItem grow={false}>
<EuiFlexGroup alignItems="center" gutterSize="s">
<EuiFlexItem grow={false}>
<AssetManager />
</EuiFlexItem>
<EuiFlexItem grow={false}>
<EuiButton
fill
size="s"
iconType="vector"
onClick={() => setShowElementModal(true)}
>
Add element
</EuiButton>
</EuiFlexItem>
</EuiFlexGroup>
</EuiFlexItem>
) : null}
</EuiFlexGroup>
</div>
);
}
}

View file

@ -7,14 +7,21 @@
import { duplicatePage } from '../actions/pages';
import { fetchRenderable } from '../actions/elements';
import { setWriteable } from '../actions/workpad';
import { getPages, isWriteable } from '../selectors/workpad';
import { getPages, getWorkpadName, isWriteable } from '../selectors/workpad';
import { getWindow } from '../../lib/get_window';
import { setDocTitle } from '../../lib/doc_title';
export const workpadUpdate = ({ dispatch, getState }) => next => action => {
const oldIsWriteable = isWriteable(getState());
const oldName = getWorkpadName(getState());
next(action);
// This middleware updates the page title when the workpad name changes
if (getWorkpadName(getState()) !== oldName) {
setDocTitle(getWorkpadName(getState()));
}
// This middleware fetches all of the renderable elements on new, duplicate page
if (action.type === duplicatePage.toString()) {
// When a page has been duplicated, it will be added as the last page, so fetch it
@ -22,17 +29,16 @@ export const workpadUpdate = ({ dispatch, getState }) => next => action => {
const newPage = pages[pages.length - 1];
// For each element on that page, dispatch the action to update it
return newPage.elements.forEach(element => dispatch(fetchRenderable(element)));
newPage.elements.forEach(element => dispatch(fetchRenderable(element)));
}
// This middleware clears any page selection when the writeable mode changes
if (action.type === setWriteable.toString() && oldIsWriteable !== isWriteable(getState())) {
const win = getWindow();
if (typeof win.getSelection !== 'function') {
return;
// check for browser feature before using it
if (typeof win.getSelection === 'function') {
win.getSelection().collapse(document.querySelector('body'), 0);
}
win.getSelection().collapse(document.querySelector('body'), 0);
}
};