From 1a05ea594401e3c4c7fc2bbcfe1e169a0bb87dc7 Mon Sep 17 00:00:00 2001 From: Robert Monfera Date: Thu, 27 Dec 2018 19:56:48 +0100 Subject: [PATCH] [Grouping] improvements (#27581) * chore: remove asynchronous element selection * chore: remove duplication between duplicateElement and rawDuplicateElement * chore: renamed rawDuplicateElement to insertNodes (pure rename, not yet set based) * perf: bulk node insertion * chore: removed unnecessary element id duplications * fix: supply empty array if preexisting workbook had none --- .../canvas/public/components/sidebar/index.js | 13 +++-- .../public/components/workpad_page/index.js | 29 +++++++--- .../canvas/public/state/actions/elements.js | 54 +++++-------------- .../public/state/middleware/aeroelastic.js | 6 +-- .../canvas/public/state/reducers/elements.js | 27 ++++------ .../canvas/public/state/reducers/pages.js | 2 +- 6 files changed, 58 insertions(+), 73 deletions(-) diff --git a/x-pack/plugins/canvas/public/components/sidebar/index.js b/x-pack/plugins/canvas/public/components/sidebar/index.js index 5cdbc8668ce55..8fe520f971211 100644 --- a/x-pack/plugins/canvas/public/components/sidebar/index.js +++ b/x-pack/plugins/canvas/public/components/sidebar/index.js @@ -5,8 +5,10 @@ */ import { connect } from 'react-redux'; -import { duplicateElement, elementLayer } from '../../state/actions/elements'; +import { cloneSubgraphs } from '../../lib/clone_subgraphs'; +import { insertNodes, elementLayer } from '../../state/actions/elements'; import { getSelectedPage, getSelectedElement } from '../../state/selectors/workpad'; +import { selectElement } from './../../state/actions/transient'; import { Sidebar as Component } from './sidebar'; @@ -16,8 +18,13 @@ const mapStateToProps = state => ({ }); const mapDispatchToProps = dispatch => ({ - duplicateElement: (pageId, selectedElement) => () => - dispatch(duplicateElement(selectedElement, pageId)), + duplicateElement: (pageId, selectedElement) => () => { + // gradually unifying code with copy/paste + // todo: more unification w/ copy/paste; group cloning + const newElements = cloneSubgraphs([selectedElement]); + dispatch(insertNodes(newElements, pageId)); + dispatch(selectElement(newElements[0].id)); + }, elementLayer: (pageId, selectedElement) => movement => dispatch( elementLayer({ diff --git a/x-pack/plugins/canvas/public/components/workpad_page/index.js b/x-pack/plugins/canvas/public/components/workpad_page/index.js index c48326896a000..bd20220300f5d 100644 --- a/x-pack/plugins/canvas/public/components/workpad_page/index.js +++ b/x-pack/plugins/canvas/public/components/workpad_page/index.js @@ -11,12 +11,13 @@ import { notify } from '../../lib/notify'; import { aeroelastic } from '../../lib/aeroelastic_kibana'; import { setClipboardData, getClipboardData } from '../../lib/clipboard'; import { cloneSubgraphs } from '../../lib/clone_subgraphs'; -import { removeElements, rawDuplicateElement } from '../../state/actions/elements'; +import { removeElements, insertNodes } from '../../state/actions/elements'; import { getFullscreen, canUserWrite } from '../../state/selectors/app'; import { getNodes, isWriteable } from '../../state/selectors/workpad'; import { flatten } from '../../lib/aeroelastic/functional'; import { withEventHandlers } from './event_handlers'; import { WorkpadPage as Component } from './workpad_page'; +import { selectElement } from './../../state/actions/transient'; const mapStateToProps = (state, ownProps) => { return { @@ -27,9 +28,9 @@ const mapStateToProps = (state, ownProps) => { const mapDispatchToProps = dispatch => { return { - rawDuplicateElement: pageId => (selectedElement, root) => - dispatch(rawDuplicateElement(selectedElement, pageId, root)), + insertNodes: pageId => selectedElements => dispatch(insertNodes(selectedElements, pageId)), removeElements: pageId => elementIds => dispatch(removeElements(elementIds, pageId)), + selectElement: selectedElement => dispatch(selectElement(selectedElement)), }; }; @@ -80,8 +81,9 @@ export const WorkpadPage = compose( setUpdateCount, page, elements: pageElements, - rawDuplicateElement, + insertNodes, removeElements, + selectElement, }) => { const { shapes, selectedPrimaryShapes = [], cursor } = aeroelastic.getStore( page.id @@ -150,12 +152,23 @@ export const WorkpadPage = compose( }, pasteElements: () => { const { selectedElements, rootShapes } = JSON.parse(getClipboardData()); - const indices = rootShapes.map(r => selectedElements.findIndex(s => s.id === r)); const clonedElements = selectedElements && cloneSubgraphs(selectedElements); if (clonedElements) { - clonedElements.map((element, index) => - rawDuplicateElement(page.id)(element, indices.indexOf(index) >= 0) - ); + // first clone and persist the new node(s) + insertNodes(page.id)(clonedElements); + // then select the cloned node + if (rootShapes.length) { + if (selectedElements.length > 1) { + // adHocGroup branch (currently, pasting will leave only the 1st element selected, rather than forming a + // new adHocGroup - todo) + selectElement(clonedElements[0].id); + } else { + // single element or single persistentGroup branch + selectElement( + clonedElements[selectedElements.findIndex(s => s.id === rootShapes[0])].id + ); + } + } } }, }; diff --git a/x-pack/plugins/canvas/public/state/actions/elements.js b/x-pack/plugins/canvas/public/state/actions/elements.js index 960a0ddc6db40..f6a4a16db7433 100644 --- a/x-pack/plugins/canvas/public/state/actions/elements.js +++ b/x-pack/plugins/canvas/public/state/actions/elements.js @@ -192,51 +192,23 @@ export const fetchAllRenderables = createThunk( } ); -export const duplicateElement = createThunk( - 'duplicateElement', - ({ dispatch, type }, element, pageId) => { - const newElement = { ...getDefaultElement(), ...getBareElement(element) }; - // move the element so users can see that it was added +export const insertNodes = createThunk('insertNodes', ({ dispatch, type }, elements, pageId) => { + const _insertNodes = createAction(type); + const newElements = elements.map(cloneDeep); + // move the root element so users can see that it was added + newElements.forEach(newElement => { newElement.position.top = newElement.position.top + 10; newElement.position.left = newElement.position.left + 10; - const _duplicateElement = createAction(type); - dispatch(_duplicateElement({ pageId, element: newElement })); + }); + dispatch(_insertNodes({ pageId, elements: newElements })); - // refresh all elements if there's a filter, otherwise just render the new element - if (element.filter) { - dispatch(fetchAllRenderables()); - } else { - dispatch(fetchRenderable(newElement)); - } - - // select the new element - dispatch(selectElement(newElement.id)); - } -); - -export const rawDuplicateElement = createThunk( - 'rawDuplicateElement', - ({ dispatch, type }, element, pageId, root) => { - const newElement = cloneDeep(element); - // move the root element so users can see that it was added - newElement.position.top = newElement.position.top + 10; - newElement.position.left = newElement.position.left + 10; - const _rawDuplicateElement = createAction(type); - dispatch(_rawDuplicateElement({ pageId, element: newElement })); - - // refresh all elements if there's a filter, otherwise just render the new element - if (element.filter) { - dispatch(fetchAllRenderables()); - } else { - dispatch(fetchRenderable(newElement)); - } - - // select the new element - if (root) { - window.setTimeout(() => dispatch(selectElement(newElement.id))); - } + // refresh all elements just once per `insertNodes call` if there's a filter on any, otherwise just render the new element + if (elements.some(element => element.filter)) { + dispatch(fetchAllRenderables()); + } else { + newElements.forEach(newElement => dispatch(fetchRenderable(newElement))); } -); +}); export const removeElements = createThunk( 'removeElements', diff --git a/x-pack/plugins/canvas/public/state/middleware/aeroelastic.js b/x-pack/plugins/canvas/public/state/middleware/aeroelastic.js index 3d66a8f4d85b8..ac69230861f3e 100644 --- a/x-pack/plugins/canvas/public/state/middleware/aeroelastic.js +++ b/x-pack/plugins/canvas/public/state/middleware/aeroelastic.js @@ -12,8 +12,7 @@ import defaultConfiguration from '../../lib/aeroelastic/config'; import { addElement, removeElements, - duplicateElement, - rawDuplicateElement, + insertNodes, elementLayer, setMultiplePositions, fetchAllRenderables, @@ -332,8 +331,7 @@ export const aeroelastic = ({ dispatch, getState }) => { case removeElements.toString(): case addElement.toString(): - case duplicateElement.toString(): - case rawDuplicateElement.toString(): + case insertNodes.toString(): case elementLayer.toString(): case setMultiplePositions.toString(): const page = getSelectedPage(getState()); diff --git a/x-pack/plugins/canvas/public/state/reducers/elements.js b/x-pack/plugins/canvas/public/state/reducers/elements.js index 3aecaeb7c2d69..cc62a202a9882 100644 --- a/x-pack/plugins/canvas/public/state/reducers/elements.js +++ b/x-pack/plugins/canvas/public/state/reducers/elements.js @@ -10,6 +10,7 @@ import { get } from 'lodash'; import * as actions from '../actions/elements'; const getLocation = type => (type === 'group' ? 'groups' : 'elements'); +const firstOccurrence = (element, index, array) => array.indexOf(element) === index; const getLocationFromIds = (workpadState, pageId, nodeId) => { const page = workpadState.pages.find(p => p.id === pageId); @@ -122,26 +123,19 @@ export const elementsReducer = handleActions( trimElement(element) ); }, - [actions.duplicateElement]: (workpadState, { payload: { pageId, element } }) => { + [actions.insertNodes]: (workpadState, { payload: { pageId, elements } }) => { const pageIndex = getPageIndexById(workpadState, pageId); if (pageIndex < 0) { return workpadState; } - return push( - workpadState, - ['pages', pageIndex, getLocation(element.position.type)], - trimElement(element) - ); - }, - [actions.rawDuplicateElement]: (workpadState, { payload: { pageId, element } }) => { - const pageIndex = getPageIndexById(workpadState, pageId); - if (pageIndex < 0) { - return workpadState; - } - return push( - workpadState, - ['pages', pageIndex, getLocation(element.position.type)], - trimElement(element) + return elements.reduce( + (state, element) => + push( + state, + ['pages', pageIndex, getLocation(element.position.type)], + trimElement(element) + ), + workpadState ); }, [actions.removeElements]: (workpadState, { payload: { pageId, elementIds } }) => { @@ -151,6 +145,7 @@ export const elementsReducer = handleActions( } const nodeIndices = elementIds + .filter(firstOccurrence) .map(nodeId => { const location = getLocationFromIds(workpadState, pageId, nodeId); return { diff --git a/x-pack/plugins/canvas/public/state/reducers/pages.js b/x-pack/plugins/canvas/public/state/reducers/pages.js index 2ad01faafbd37..410da335e62fe 100644 --- a/x-pack/plugins/canvas/public/state/reducers/pages.js +++ b/x-pack/plugins/canvas/public/state/reducers/pages.js @@ -31,7 +31,7 @@ function clonePage(page) { // TODO: would be nice if we could more reliably know which parameters need to get a unique id // this makes a pretty big assumption about the shape of the page object const elements = page.elements; - const groups = page.groups; + const groups = page.groups || []; const nodes = elements.concat(groups); const newNodes = cloneSubgraphs(nodes); return {