Skip to content

Commit

Permalink
Fix: removes extra page re-rendering (elastic#882)
Browse files Browse the repository at this point in the history
* chore: remove ast stripping code from getPages selector

legacy code that was causing re-rendering

* fix: WorkpadPage gets elements from state

if it only relies on the elements in the page object that is passed in, it doesn't re-render when elements change in the state

* chore: remove unused dispatchers
  • Loading branch information
w33ble authored and Rashid Khan committed Jul 31, 2018
1 parent c7c3497 commit e591911
Show file tree
Hide file tree
Showing 3 changed files with 10 additions and 35 deletions.
32 changes: 8 additions & 24 deletions public/components/workpad_page/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,38 +2,22 @@ import { connect } from 'react-redux';
import PropTypes from 'prop-types';
import { compose, withState, withProps } from 'recompose';
import { aeroelastic } from '../../lib/aeroelastic_kibana';
import { removeElement, setPosition } from '../../state/actions/elements';
import { selectElement } from '../../state/actions/transient';
import { removeElement } from '../../state/actions/elements';
import { getFullscreen, getEditing } from '../../state/selectors/app';
import { getElements } from '../../state/selectors/workpad';
import { withEventHandlers } from './event_handlers';
import { WorkpadPage as Component } from './workpad_page';

const mapStateToProps = state => {
const mapStateToProps = (state, ownProps) => {
return {
isFullscreen: getFullscreen(state),
isEditing: getEditing(state),
isEditable: !getFullscreen(state) && getEditing(state),
elements: getElements(state, ownProps.page.id),
};
};

const mapDispatchToProps = dispatch => {
return {
removeElement: pageId => elementId => dispatch(removeElement(elementId, pageId)),
selectElement: isInteractable => elementId =>
isInteractable && dispatch(selectElement(elementId)),
setPosition: pageId => (elementId, position) =>
dispatch(setPosition(elementId, pageId, position)),
};
};

const mergeProps = (stateProps, { removeElement }, ownProps) => {
const { isEditing, isFullscreen } = stateProps;
const { page } = ownProps;

return {
...ownProps,
isEditable: !isFullscreen && isEditing,
key: page.id,
removeElement,
};
};

Expand All @@ -46,11 +30,11 @@ const getRootElementId = (lookup, id) => {
};

export const WorkpadPage = compose(
connect(mapStateToProps, mapDispatchToProps, mergeProps),
connect(mapStateToProps, mapDispatchToProps),
withState('updateCount', 'setUpdateCount', 0), // TODO: remove this, see setUpdateCount below
withProps(({ updateCount, setUpdateCount, page, removeElement }) => {
withProps(({ updateCount, setUpdateCount, page, elements: pageElements, removeElement }) => {
const { shapes, selectedShapes = [], cursor } = aeroelastic.getStore(page.id).currentScene;
const elementLookup = new Map(page.elements.map(element => [element.id, element]));
const elementLookup = new Map(pageElements.map(element => [element.id, element]));
const shapeLookup = new Map(shapes.map(shape => [shape.id, shape]));
const elements = shapes.map(
shape =>
Expand Down
3 changes: 1 addition & 2 deletions public/components/workpad_page/workpad_page.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ export const WorkpadPage = ({
WorkpadPage.propTypes = {
page: PropTypes.shape({
id: PropTypes.string.isRequired,
style: PropTypes.object,
}).isRequired,
elements: PropTypes.arrayOf(
PropTypes.shape({
Expand All @@ -98,13 +99,11 @@ WorkpadPage.propTypes = {
isSelected: PropTypes.bool.isRequired,
height: PropTypes.number.isRequired,
width: PropTypes.number.isRequired,
style: PropTypes.object,
isEditable: PropTypes.bool.isRequired,
onDoubleClick: PropTypes.func,
onKeyDown: PropTypes.func,
onKeyUp: PropTypes.func,
onMouseDown: PropTypes.func,
onMouseMove: PropTypes.func,
onMouseUp: PropTypes.func,
shapes: PropTypes.arrayOf(PropTypes.object),
};
10 changes: 1 addition & 9 deletions public/state/selectors/workpad.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,7 @@ export function getSelectedPage(state) {
}

export function getPages(state) {
const pages = get(state, append(workpadRoot, 'pages'), []);

// explicitely strip the ast, basically a fix for corrupted workpads
// due to https://github.com/elastic/kibana-canvas/issues/260
// TODO: remove this once it's been in the wild a bit
return pages.map(page => ({
...page,
elements: page.elements.map(el => omit(el, ['ast'])),
}));
return get(state, append(workpadRoot, 'pages'), []);
}

export function getPageById(state, id) {
Expand Down

0 comments on commit e591911

Please sign in to comment.