[Canvas] Layout engine integration simplification #33702

monfera · 2019-03-22T12:40:27Z

Goals:

Minimize dependence on Redux (eg. by minimal or no layout middleware and by putting the layout state into Workpad or WorkpadPage component state) to allow alternative data flow concepts eg. Flux, Vuex, FRP, anything
Remove the awkwardness of the Redux Store - React WorkpadPage - Layout Engine association, eg. the Map in aeroelastic_kibana.js that herds a set of layout stores
General refactoring goals: more approachable, testable, TSable, regular code
Component event handlers should have minimal access to layout engine props (eg. to make @cqliu1's context menu, SavedObjects and further keyboard shortcut integration simpler)
Unblock progress on bug fixes and feature PRs eg. box select
More consistent naming (eg. *Elements -> *Nodes where appropriate; old PR feedback from @w33ble )

Side effects:

more compact integration code (-360 lines)
lighter on resources:
- only the ever current page has event handlers
- less work done on high FPS continuous operations eg. element drag
- unrelated upstream prop changes don't cause rerenders (shouldUpdate)
- the layout engine comes into play for the current page only; no state initialization per page when loading
- less code to run even for the current page
fixes [Canvas] Grouping while holding down the mouse key damages the elements #34413 (bug found after the creation of this PR)
closes [Canvas] Persist grouping of multiple selected elements elements when pasted #29656
closes [Canvas] Resize/rotate then Z-reorder loses element border #29946
closes [Canvas][Layout Engine] It may take two clicks to select a group #30785
closes [Layout Engine] Sometimes selecting an element for ad-hoc grouping didn't succeed #26440 (this issue didn't reproduce, but it's unlikely the new code has the same bug)
unblocks [Canvas] Feat: Box select #32995 and other new interaction features

Non-goals for this specific PR:

fixing bugs (some arose due to convoluted code; others can be fixed with small PRs)
adding features (some seemingly simple features eg. box select were in effect blocked)
increasing speed, TS use and test coverage (doing code regularization first has some pros)
doing ALL the envisioned refactoring (further work can be done more incrementally)
refactoring or significantly touching the layout engine itself (there are no reported bugs or performance issues, but all contributors would benefit from code reduction)

Progress:

elasticmachine · 2019-03-22T12:40:41Z

Pinging @elastic/kibana-canvas

elasticmachine · 2019-03-22T13:28:29Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-03-31T12:13:01Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-08T14:11:55Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

w33ble

Initial feedback, have not run this yet.

nit: nitpicky stuff you can choose to ignore
Q: question
CR: change requested

also curious if you have plans to slim down the WorkpadPage component in the future, it's a beast now...

x-pack/plugins/canvas/public/state/middleware/workpad_update.js

w33ble · 2019-04-08T17:10:40Z

x-pack/plugins/canvas/public/apps/export/routes.js

@@ -32,6 +33,7 @@ export const routes = [
          dispatch(setAssets(assets));
          dispatch(setWorkpad(workpad, { loadPages: false }));
          dispatch(setPage(pageNumber - 1));
+          dispatch(selectToplevelNodes([]));


Q: Was this just an oversight in the old code, where selected elements were not cleared?

In the old code, we cleared the selection via the middleware (I think circuitously, by clearing it in the aero store which then updated back redux) so it just used a different - and simpler - mechanism there. But glad you asked because:

I'm not proposing this for this PR, but adding this line (and similar ones) made me ponder if our currently fairly lean reducers should be able to do more. For example, here, we issue four dispatches. combineReducers is nice when state slices are truly independent, but it's not possible to implement a new version of setPage that would also vacate the transient.selectedToplevelNodes array. I'm biased toward one user action = one Redux action (sans async stuff like server response) and the Redux doc has a pertinent note on combineReducers.

I also prefer to lean towards the 1:1 user action to redux action route.

Would it make sense to update the transientReducer to be watching for any of these "page changed" type actions (add, set, delete, duplicate, etc) and set the selectedTopLevelNodes state to empty array? Rather than having to always dispatch this selectTopLevelNodes action when any kind of page change happens?

if (action is changePage or deletePage or addPage) selectedTopLevelElements = []

That's probably the better "redux" way, having the transient reducer update based on these actions. So that dispatching only setWorkpad or setPage does what you want without needing to dispatch other actions all the time.

There's a lot of other places we could clean up like that too.

x-pack/plugins/canvas/public/components/workpad_page/event_handlers.js

x-pack/plugins/canvas/public/components/workpad_page/index.js

x-pack/plugins/canvas/public/state/selectors/__tests__/workpad.js

x-pack/plugins/canvas/public/state/selectors/workpad.js

monfera · 2019-04-08T19:11:50Z

also curious if you have plans to slim down the WorkpadPage component in the future, it's a beast now

Yes, agreed! Should I put the wrappers around the (already split) InteractiveWorkpadPage and StaticWorkpadPage in a separate component, in effect, splitting the workpad_page directory into three (one simple, one interactive and one for branching between the two)? As part of this PR? I haven't yet done it on the offchance that this kind of separation (to simple vs interactive) is frowned upon for some technical reason.

w33ble · 2019-04-08T19:19:09Z

Should I put the wrappers around the (already split) InteractiveWorkpadPage and StaticWorkpadPage in a separate component, in effect, splitting the workpad_page directory into three (one simple, one interactive and one for branching between the two)? As part of this PR? I haven't yet done it on the offchance that this kind of separation (to simple vs interactive) is frowned upon for some technical reason.

I'm not sure the best way to spit it up, and personally would not do it in this PR anyway. Was just curious if you had future plans. Sounds like yes, which is good enough for me.

crob611 · 2019-04-08T19:57:26Z

x-pack/plugins/canvas/public/components/page_manager/index.js

+  },
+  removePage: id => {
+    dispatch(pageActions.removePage(id));
+    dispatch(selectToplevelNodes([]));


Minor: If the page being removed is not the one that's currently the interactive one, then the selections are still reset.

Good catch, thanks!

elasticmachine · 2019-04-09T13:28:01Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

w33ble

LGTM. It would be nice if you removed all the added dispatch(selectToplevelNodes([])) and put that logic in the transient reducer, but I'm not going to block on that.

… ops" This reverts commit aa25653

…ection for removePage

elasticmachine · 2019-04-11T18:32:43Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* Refactor: layout engine integration

elasticmachine · 2019-04-14T11:35:52Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

monfera added the WIP Work in progress label Mar 22, 2019

monfera self-assigned this Mar 22, 2019

monfera requested a review from a team as a code owner March 22, 2019 12:40

monfera added the Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas label Mar 22, 2019

monfera changed the title ~~[Canvas][WIP] Layout state simplification~~ [Canvas][WIP][skip ci] Layout state simplification Mar 22, 2019

monfera force-pushed the state-simplification branch from d9a1689 to bd3af51 Compare March 22, 2019 21:26

monfera force-pushed the state-simplification branch 2 times, most recently from ace6772 to 7d0f141 Compare March 31, 2019 08:34

monfera changed the title ~~[Canvas][WIP][skip ci] Layout state simplification~~ [Canvas][WIP] Layout state simplification Mar 31, 2019

monfera force-pushed the state-simplification branch 3 times, most recently from b41778e to a3688f3 Compare March 31, 2019 11:33

elastic deleted a comment from elasticmachine Mar 31, 2019

monfera requested review from w33ble and cqliu1 March 31, 2019 12:38

monfera added review v7.0.0 v8.0.0 labels Mar 31, 2019

monfera mentioned this pull request Mar 31, 2019

[Canvas] Feat: Box select #32995

Merged

monfera changed the title ~~[Canvas][WIP] Layout state simplification~~ [Canvas][WIP] Layout engine integration simplification Mar 31, 2019

monfera requested a review from clintandrewhall March 31, 2019 12:58

This was referenced Apr 3, 2019

[Canvas] Grouping while holding down the mouse key damages the elements #34413

Closed

[Canvas] Deleting the last extant page of a workbook throws #34458

Closed

monfera force-pushed the state-simplification branch from a3688f3 to 1a4988c Compare April 8, 2019 13:30

w33ble suggested changes Apr 8, 2019

View reviewed changes

crob611 reviewed Apr 8, 2019

View reviewed changes

monfera added 4 commits April 9, 2019 02:28

Refactor: layout engine integration

c78c1e0

Fix: erase selectedToplevelNodes from state upon certain page ops

aa25653

Fix: page miniviews

3d196ff

Chore: safeguard against missing commit

49f95b4

monfera force-pushed the state-simplification branch from 1a4988c to 49f95b4 Compare April 9, 2019 12:43

w33ble approved these changes Apr 9, 2019

View reviewed changes

snide removed v7.0.0 labels Apr 10, 2019

w33ble added the v7.2.0 label Apr 11, 2019

monfera added 3 commits April 11, 2019 19:00

Revert "Fix: erase selectedToplevelNodes from state upon certain page…

c36f770

… ops" This reverts commit aa25653

Chore: no need to change state if we're already on the required page

d3af3e5

Fix: action-centric way for removing selections; no need to reset sel…

92a3bc9

…ection for removePage

monfera changed the title ~~[Canvas][WIP] Layout engine integration simplification~~ [Canvas] Layout engine integration simplification Apr 11, 2019

monfera merged commit b7871a5 into elastic:master Apr 11, 2019

monfera mentioned this pull request Apr 11, 2019

[7.x] [Canvas] Layout engine integration simplification (#33702) #34958

Merged

monfera added a commit to monfera/kibana that referenced this pull request Apr 11, 2019

[Canvas] Layout engine integration simplification (elastic#33702)

769d11c

* Refactor: layout engine integration

monfera added a commit that referenced this pull request Apr 11, 2019

[Canvas] Layout engine integration simplification (#33702) (#34958)

ffa538e

* Refactor: layout engine integration

monfera mentioned this pull request Apr 12, 2019

[Canvas] component/-level separation of workpad_page #34983

Merged

monfera mentioned this pull request Apr 16, 2019

[Canvas][Layout Engine][WIP][skip ci] Refactor: Rework of Layout Engine state #30827

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Canvas] Layout engine integration simplification #33702

[Canvas] Layout engine integration simplification #33702

monfera commented Mar 22, 2019 •

edited

Loading

elasticmachine commented Mar 22, 2019

elasticmachine commented Mar 22, 2019

elasticmachine commented Mar 31, 2019

elasticmachine commented Apr 8, 2019

w33ble left a comment

w33ble Apr 8, 2019

monfera Apr 8, 2019 •

edited

Loading

crob611 Apr 8, 2019

w33ble Apr 9, 2019

monfera commented Apr 8, 2019

w33ble commented Apr 8, 2019

crob611 Apr 8, 2019

monfera Apr 8, 2019

elasticmachine commented Apr 9, 2019

w33ble left a comment

elasticmachine commented Apr 11, 2019

elasticmachine commented Apr 14, 2019

[Canvas] Layout engine integration simplification #33702

[Canvas] Layout engine integration simplification #33702

Conversation

monfera commented Mar 22, 2019 • edited Loading

elasticmachine commented Mar 22, 2019

elasticmachine commented Mar 22, 2019

💚 Build Succeeded

elasticmachine commented Mar 31, 2019

💚 Build Succeeded

elasticmachine commented Apr 8, 2019

💚 Build Succeeded

w33ble left a comment

Choose a reason for hiding this comment

w33ble Apr 8, 2019

Choose a reason for hiding this comment

monfera Apr 8, 2019 • edited Loading

Choose a reason for hiding this comment

crob611 Apr 8, 2019

Choose a reason for hiding this comment

w33ble Apr 9, 2019

Choose a reason for hiding this comment

monfera commented Apr 8, 2019

w33ble commented Apr 8, 2019

crob611 Apr 8, 2019

Choose a reason for hiding this comment

monfera Apr 8, 2019

Choose a reason for hiding this comment

elasticmachine commented Apr 9, 2019

💚 Build Succeeded

w33ble left a comment

Choose a reason for hiding this comment

elasticmachine commented Apr 11, 2019

💚 Build Succeeded

elasticmachine commented Apr 14, 2019

💚 Build Succeeded

monfera commented Mar 22, 2019 •

edited

Loading

monfera Apr 8, 2019 •

edited

Loading