[Canvas][Layout Engine] Persistent grouping #25854

monfera · 2018-11-19T08:56:08Z

Summary

Add persistent grouping and ungrouping to Canvas, covering the core functionality of grouping.

Group / ungroup changes:

Select multiple elements with shift-click and group with key g for persistent grouping
Select a persistent group and break it up with key u
Group hierarchically: a group can have other groups as constituents
Group can be rotated like an element
Group can be resized like an element, enabled only when all elements are rotated at multiples of 90 degrees (ie. axis aligned relative to the canvas)
The group resize will resize the contents appropriately, possibly causing axis tick counts etc. to change in element contents in response to their new relative size (it preserves horizontal / vertical ratios)
Group delete: hierarchically deletes all elements in the group incl. other groups

Rough edges to be fixed in this PR:

Sometimes the element needs to be selected one more time before ungrouping succeeds
Despite disabling resizing except for canvas axis aligned elements, it's still possible to skew elements by combinations of grouping, rotation and resize - don't allow this for now
Hierarchical grouping (ie. grouping a group) and subsequent resizing will not appropriately move and resize leaf contents - either solve it or disable resizing for groups of groups for now
Sometimes an additional element can't be selected for group inclusion because the group bounding box already overlaps it

Tasks arising from PR feedback:

Persist only minimal grouping info, as opposed to persisting the resulting element tree

Planned improvements, as subsequent small PRs:

When elements to group are not axis aligned relative to the canvas, but all of them are axis aligned relative to one another, wrap them with a similarly axis aligned box, so that 1) resizing can be preserved; 2) the entire group can be more easily rotated to an exact horizontal/vertical position; 3) grouping canvas axis aligned elements, then rotating them, then ungrouping, then grouping them again works intuitively
Group boundaries get altered after a group-resize-reload cycle
Correct proportional resizing with groups of groups
Add Group/Ungroup buttons next to eg. Clone (an eventual context menu is a theme for a different gh issue)

Follow-up work:

[Canvas] Element templates #25531

Internal changes related to grouping:

Persist the group in Redux and on the server (via a new element and a parent property
The parent property is persisted in Redux and the index
Instead of using transformMatrix, work with localTransformMatrix in client aeroelastic.js

Other internal changes:

Add configuration object instead of hardcoding configuration into aeroelastic (original feedback on the layout engine from @w33ble)
Other unrelated improvements (renames; abstracting out some functions etc.)

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, except a check against IE11
This was checked against IE11
~~- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
Documentation was added for features that require explanation or tutorials
~~- [ ] Unit or functional tests were updated or added to match the most common scenarios~~
This was checked for keyboard-only and screenreader accessibility

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

elasticmachine · 2018-11-19T11:09:31Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

monfera · 2018-11-19T17:21:51Z

Element content control when grouped:

Resize is proportional; scrollbar functional too:

X/Y resize for a responsive dashboard emulation:

Centered resize (Option key) looks trippy:

elasticmachine · 2018-11-19T17:26:26Z

Pinging @elastic/kibana-canvas

monfera · 2018-11-20T07:57:23Z

@rashidkpc @w33ble a question on migration: should we prepare for 7.x workpads being opened with v6.5? I just tested it, and positioning is fine, one reason for which we store the absolute positions (ie. works fine in presentation mode), except of course that in edit mode, moving the group will just move an empty frame, the contents stay put. Which one should we do?

not a concern
put the new 'grouping' element in some new part of the Redux state and index so that an old version just gets the leaf nodes, ie. it'll open the workpad, except groups fall apart to their leaf nodes (any interaction will then make the workpad re-persist in the index, and .... hmmm... I guess the grouping part of the index would remain intact? can lead to inconsistent things once the user opens it again with 7.x or if they ever upgrade in the future)

Doing option 1 would be comforting in that we don't tamper with indices - I think that leaving an invisible group border "residue" (like a completely transparent element) is the smaller bad - visible and transparent handling, haha - compared to leaving stuff in the index and revisiting with v7.x will produce who knows what. Are there stronger reasons for doing something else?

w33ble · 2018-11-20T19:51:18Z

@monfera we have a schemaVersion in the state, which gets exported with the workpad. We should bump the value with the grouping stuff it sounds like. We also still need to add code to deal with that value.

I think it's reasonable to treat workpads with a new version differently in 6.x. What we do with that is probably up for debate, but I think option 2 makes sense. Things would still function at least, even if you lose the grouping stuff when you go backwards. It's really @rashidkpc's call though.

rashidkpc · 2018-11-20T21:17:52Z

I know there's still work to do on this, but I wanted to weigh in on some architecture concerns here. In general we want to keep persisted document changes to the absolute minimum required to achieve a piece of user facing functionality. The less things we store, the fewer opportunities for breaking changes. We also strive to keep our schema extremely human readable. This makes it easier for users to understand and tweak.

Here's what a page array looked like before this:

{
  "id": "element-1",
  "position": {
    "left": 0,
    "top": 0,
    "width": 500,
    "height": 300,
    "angle": 0
  },
  "expression": "demodata type=ci"
},
{
  "id": "element-2",
  "position": {
    "left": 500,
    "top": 0,
    "width": 500,
    "height": 300,
    "angle": 0
  },
  "expression": "demodata type=shirts"
},

And after, with the previous 2 elements grouped:

{
  "id": "element-1",
  "position": {
    "left": 0,
    "top": 0,
    "width": 500,
    "height": 300,
    "angle": 0,
    "parent": "group_792858350394",
    "localTransformMatrix": [
      1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1, 0, -250, 0, 0, 1
    ]
  },
  "expression": "demodata type=ci"
},
{
  "id": "element-2",
  "position": {
    "left": 500,
    "top": 0,
    "width": 500,
    "height": 300,
    "angle": 0,
    "parent": "group_792858350394",
    "localTransformMatrix": [
      1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1, 0, 250, 0, 1, 1
    ]
  },
  "expression": "demodata type=shirts"
},
{
  "id": "group_792858350394",
  "position": {
    "left": 0,
    "top": 0,
    "width": 1000,
    "height": 300,
    "angle": 0,
    "parent": null,
    "localTransformMatrix": [
      1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1, 0, 500, 150, 0, 1
    ]
  },
  "expression": "shape fill=\"rgba(255,255,255,0)\" | render"
}

Only 1 change is being intentionally being created by the user: The creation of the group.
But we're creating 3 schema level changes:

The concept of an element with an id starting with "group". This is sort of a meta element, but is still expression driven.
The parent property
The localTransformMatrix property. This property is also being created for elements that aren't grouped.

Let's tweak this pull to only store a single change: parent, which could be called group to avoid confusion. We can then auto-generate the other bits of state as transient properties upon loading of the workpad. That would avoid introducing the duals concepts of the meta grouping element, and the addition of the localTransformMatrix property.

It would have the added benefit of not causing any immediately observable issues when loading a grouping enabled workpad in a Canvas installation that isn't grouping enabled, the elements would simply fail to be grouped and would lose their grouped status if manipulated.

This would leave us with the new persisted elements, grouped, appearing as such:

{
  "id": "element-1",
  "position": {
    "left": 0,
    "top": 0,
    "width": 500,
    "height": 300,
    "angle": 0,
    "group": "group-1"
  },
  "expression": "demodata type=ci"
},
{
  "id": "element-2",
  "position": {
    "left": 500,
    "top": 0,
    "width": 500,
    "height": 300,
    "angle": 0,
    "group": "group-1"
  },
  "expression": "demodata type=shirts"
},

monfera · 2018-11-20T22:32:40Z

@rashidkpc @w33ble thanks for the feedback!

I'll strive to automatically generating things, if I understand, it'll mean that we don't persist in the index some stuff that's present in redux. I have to think it through with this goal in mind. There are lots of positives about it, thanks also for enlisting many of those.

As it's late here, I just jot down quick thoughts that are also consequences of the compressed representation in the index, glad to receive comments for tomorrow if possible. It's not an appeal for avoiding change, more like sharing thoughts that arise during work. Summary at the end.

Consequences of minimizing index level storage changes:

Single level hierarchies can be done with a simple parent property: a common value across a bunch of elements implies that they belong in the same group. A group may belong to a higher level group. This would necessitate something like an "ancestor list" (array of strings) in the ancestors on the leaf element, if we don't mind the resulting complexity and the disjoint representation between index and redux (I just assumed it's good to mirror, as it seemed to be the case, but so far there was I guess only the initial version). Upshot: A bijective relationship can be had, we can map to and fro with some code.
Currently, knowing element positions within a group is enough to reconstruct the localTransformMatrix values, because we activated three things only: translation, rotation and resize (the latter of which is just props ie. we just have two special affine transforms). Resizing the group proportionally repositions and resizes the internal members, still bijective. If we eventually want to add more sophistication to it - the planned 3D stuff, zoom scaling or even just the common 2D mirroring or skew options -, then we'll need the localTransformMatrix (or similar)
Groups are first class objects in that they may have associated properties. Examples: a) a group whose elements don't just proportionally rescale, but are subject to eg. minimum or maximum width constraints, responsive style; b) CSS styling or other visual props; c) accessibility requirements using eg. ARIA text labels for group elements; d) we aren't yet sure of details, but it is likely that canvas expressions will be useful for groups as well, eg. for data driven behaviors including possibly, positioning, styling or generating their internal elements (ie. a group may not even have static elements in the index; they're generated on the fly); e) even templating might need it, or benefit from it
Now there's no server side functionality associated with groups, but in the future, things like collaborative editing (eg. User 1 highlights a group for a remote User 2) would benefit from a first-class natural representation even if the grouping info can be encoded and decoded. Upshot: these are probably some ways off, and may need other index changes anyway.
We could eventually have functionality that'd naturally collapse a group into a single element - eg. templates - or break up an element into a group - maybe breaking up an SVG or a chart, making the status between leaf and group nodes fluid (we could still do the ancestors list trick though)
While we can compress at the index level, state in Redux needs to handle groups as first class entities, for example, when selecting a group, copy/pasting etc.
We're not in GA yet - it may be easier to futureproof our storage a bit now eg. with a hierarchy, than when we're in GA and migration is a more serious concern.

To wrap it up, it feels like we can get away with an encoded representation now - even this is some kind of change to the index with the ancestors string array - but there are necessities for handling groups as 1st class scenegraph elements for things under discussion, so it boils down to, whether we want to minimize change now, or exploiting that we're not in GA yet, we consider the overall shape, if not the details, for index representation, now a tree organization that's uniform across group and leaf nodes. My concern is the hard to estimate cost of translating between two formats, it feels trivial with ancestors but also feels like accumulating implementation burden over time

It would have the added benefit of not causing any immediately observable issues when loading a grouping enabled workpad in a Canvas installation that isn't grouping enabled, the elements would simply fail to be grouped and would lose their grouped status if manipulated.

It's true it'd be possible, and desirable, even though we could draw the line at GA (ie. not promote the use of beta / pre-beta Canvas, once it's in GA) as we can go into GA only once. Even if it's a goal for now, maybe we could achieve it by saving the leaf nodes where they are, and saving the grouping tree in some new corner of the index that the current version doesn't read.

Having said all, I'm glad to go the compression route if confirmed, traversing the tree to get an ancestors list doesn't feel like a complex thing, and representation can be revisited in some future release.

elasticmachine · 2018-11-21T18:49:22Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

w33ble · 2018-11-21T21:17:55Z

it'll mean that we don't persist in the index some stuff that's present in redux

You can get around that by encapsulating calculated values in selectors. For example, the getPages selector can return the hydrated version we need in the application, but the redux state itself stays simple.

ryankeairns · 2018-11-27T19:50:00Z

I'm currently working on UI mockups for element templates and it has me thinking about the settings panel for grouped elements. For this initial version of grouped elements, we're just showing the workpad level settings in the righthand panel, so I'm wondering if/how that might change once we get into element templates (which seem to evolve from grouped elements). In other words, are there derived or defined settings for an element template or grouped elements? Can we derive settings from the underlying elements? etc.

I'll create a separate issue or add some thoughts to the element templates issue #25531 , but wanted to raise it here as it seems like a potential addition to the Planned improvements, as subsequent small PRs: section in the description of this issue.

monfera · 2018-11-27T20:35:36Z

Thanks @w33ble! The layout engine is pretty much selectors all the way through, so I guess we'd do something similar outside the layout engine, for at least the group elements, because they're needed for some stuff there too (eg. selecting / unselecting a group). There might be some duplication, maybe avoidable.

monfera · 2018-11-27T20:35:55Z

Thanks @ryankeairns! Good point, groups currently don't do anything for the side bar, the only reason I didn't add it as an item for follow-up is that as you say, it must be addressed for templates anyway, but I made a followup reference there too.

monfera · 2018-12-06T13:16:16Z

Hierarchical grouping is coming with the next rebase+push, it's needed for the responsive build-up of widgets from the bottom up (our current baseline is that all elements are resizable, it wouldn't have been nice to break it for no good reason)

(elements don't need to be adjacent btw., they can be anywhere, any gaps will resize proportionally until we eventually have more refined constraints like minimum and maximum width)

alexfrancoeur · 2018-12-06T14:21:17Z

@monfera would you like feedback this week or should I wait a bit longer?

monfera · 2018-12-06T14:29:27Z

@alexfrancoeur please hold on, there are still three checkboxes to fill in the "Rough edges to be fixed in this PR" and "Tasks arising from PR feedback" sections on the top

alexfrancoeur · 2018-12-06T15:40:52Z

Thanks Robert! I'll wait for them to o be checked

…

On Thu, Dec 6, 2018, 9:29 AM Robert Monfera ***@***.***> wrote: @alexfrancoeur <https://github.com/alexfrancoeur> please hold on, there are still three checkboxes to fill in the "Rough edges to be fixed in this PR" and "Tasks arising from PR feedback" sections on the top — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#25854 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AYA-f6kHFTH8wBZILWXqhH_x2C4YJhZ1ks5u2SnhgaJpZM4YoqDf> .

monfera · 2018-12-14T01:29:35Z

@alexfrancoeur it would be a good time to check this out, there are still two things I'm planning (1. do not select a group unless a constituent element is selected; now even if you click on an empty area in the group, it gets selected; 2. maybe buttons? now it's just G / U, not too discoverable

elasticmachine · 2018-12-14T02:26:46Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

w33ble · 2018-12-18T19:34:25Z

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

@@ -137,6 +137,16 @@ const handleKeyDown = (commit, e, isEditable, remove) => {
  }
 };

+const handleKeyPress = (commit, e, isEditable) => {


Is there a reason you're not handling this in the handleKeyDown handler? My understanding is that both handlers will be called in most cases, with alt, ctrl, shift, and meta being the exception.

MDN: The keydown event is fired when a key is pressed down. Unlike the keypress event, the keydown event is fired for keys that produce a character value and for keys that do not produce a character value.

So pressing G or U is still going to trigger this:

commit('keyboardEvent', { event: 'keyDown', code: keyCode(key), // convert to standard event code });

Maybe that's intentional?

Yes it could technically work both ways (actually, keyPress will activate on the upward trajectory), it was simply a question of showing intent - one is concerned with signaling intent with a keypress gesture - 'G' or 'U' here - while the other conveys up / down events.

The difference would be if you wanted to include control, shift, option, etc. Keypress would ignore keys combined with option on a mac, I believe.

Yes, keypress does ignore those keys. My point was that onKeyDown is always going to be called, and that internal event would always be triggered, even though those specific keys are handled differently and in a different handler. Sounds like it shouldn't cause problems though.

w33ble · 2018-12-18T19:39:11Z

x-pack/plugins/canvas/public/state/actions/elements.js

+
+    // select the new element
+    if (root) {
+      window.setTimeout(() => dispatch(selectElement(newElement.id)));


Why does this need to be wrapped in setTimeout? What happens if you dispatch immediately?

I want to circle back to this one, the element in aeroelastic is not present yet if done synchronously, so it can't select (pasting will create a new group, naturally with a new id). So the purpose is to switch selection to the newly pasted thing, even if it's a group. I think it's due to the fact that cloning is done outside aero but selection is in good part, inside (eg. to activate the resize frame). I'm sometimes thinking about a different synchronization between aero and the Redux actions / thunks, it got fairly convoluted. Something I'm planning to touch on on our Jan 10 discussion on the layout engine.

w33ble

Some small change requests mixed with some questions.

w33ble · 2018-12-18T19:48:02Z

x-pack/plugins/canvas/public/state/actions/elements.js

@@ -213,12 +214,48 @@ export const duplicateElement = createThunk(
  }
 );

+export const rawDuplicateElement = createThunk(


There's a lot of duplication of the code in duplicateElement here... can you pull that out into a function that both actions use? Or even better, just use duplicateElement with a new flag that indicates it should just clone the original object instead of doing a partial clone (afaict, that'd the difference between the two actions).

Yes, I was planning to layer it, ie. duplicateElement relying on raw..., or using an argument, or extracting out the common part (which is almost everything). I haven't done so due to lack of time and not having settled on what the best DRY option is here.

I'm cool with just opening a new issue about de-duping this code and merging this PR with it then.

Opened #27447

w33ble · 2018-12-18T19:53:33Z

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

+    .concat((get(page, 'groups') || []).map(augment('group')));
+
+// todo unify or DRY up with `getElements`
+export function getNodes(state, pageId, withAst = true) {


It looks like this is meant to replace getElements, and it also looks like nothing uses getElements anymore... can we remove that selector?

Good general point (getElements is still used eg. by getElementById which in turn is used by getSelectedElement etc. but the ultimate caller could/should switch to getNode... when we eg. want to update the sidebar from the selection, which isn't happening now)

Opened #27449

w33ble · 2018-12-18T19:53:48Z

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

  const element = getElements(state, pageId, []).find(el => el.id === id);
  if (element) {
    return appendAst(element);
  }
 }

+export function getNodeById(state, id, pageId) {


It looks like this is meant to replace getElementById, and it also looks like nothing uses getElementById anymore... can we remove that selector?

getElementById is still in use but maybe not for too long, which is why I didn't worry regularizing it for now - the group templates will definitely bring in some new aspects, which will, I think, make elements and groups more and more similar.

Yeah, it seemed like you were moving to "nodes" instead of "elements" as the terminology, so a node might be an element or a great of elements.

The only code I see that still uses getElementById right now is the tests for the selector ;)

w33ble · 2018-12-18T19:55:32Z

x-pack/plugins/canvas/public/state/initial_state.js

      // See the resolved_args reducer for more information.
    },
    persistent: {
-      schemaVersion: 1,
+      schemaVersion: 2,


If we are going to increment this, do we need any kind of migration to convert from version 1 to version 2? If things will just continue to work, do we really need to increment the schema version?

It didn't look like we neeed to increment, I just acted on an earlier feedback item, though that might have been under different assumptions.

We don't need to migrate, so this can be reverted; on the other hand, the changes do represent a schema change (new props) so I didn't question the original feedback item, looked totally sensible.

In short, it can go either way; no need for migration.

OK, might as well bump it then.

w33ble · 2018-12-18T19:58:22Z

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

-    aero.matrix.rotateZ(angleRadians)
-  );
+  const transformMatrix =
+    //position.localTransformMatrix ||


Delete this.

Done; will push

w33ble · 2018-12-18T20:16:11Z

x-pack/plugins/canvas/public/state/reducers/elements.js

+        trimElement(element)
+      );
+    },
+    [actions.rawDuplicateElement]: (workpadState, { payload: { pageId, element } }) => {


Like the action, this is duplicating a lot of code from duplicateElement (in fact, I think the two are identical). Getting back to a single action would fix it, otherwise pulling this code into a helper function would be good.

Legit point. I have larger concerns as well, which is why I didn't bother, besides of course not getting around to it 😄 For it's quite inefficient to duplicate subtrees element by element. Similar to positioning, I'm planning to switch to bulk subtree duplication and these both will just go away 😄

w33ble · 2018-12-18T20:21:42Z

x-pack/plugins/canvas/public/lib/clone_subgraphs.js

+  const idMap = arrayToMap(nodes.map(n => n.id));
+  // We simultaneously provide unique id values for all elements (across all pages)
+  // AND ensure that parent-child relationships are retained (via matching id values within page)
+  Object.entries(idMap).forEach(([key]) => (idMap[key] = getId(key.split('-')[0]))); // new group names to which we can map


Since you're discarding the value in the entry, wouldn't Object.keys(idMap).forEach(key => ... do the same thing here?

Haha good spot, I overlooked this one, apparently ES2015 zealot here 😄 Changing it now

Done, will push; an initial version used the value too and it got stuck :-)

I only dug in because I haven't used Object.entries before and had to look it up ;)

clintandrewhall

I think @w33ble and @rashidkpc have covered a lot here... my comments are all nits and best practices you may consider adding in a follow-up.

clintandrewhall · 2018-12-18T20:35:20Z

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

@@ -125,7 +125,7 @@ const handleKeyDown = (commit, e, isEditable, remove) => {
  const { key, target } = e;

  if (isEditable) {
-    if (isNotTextInput(target) && (key === 'Backspace' || key === 'Delete')) {
+    if ((key === 'Backspace' || key === 'Delete') && !isTextInput(target)) {


At some point, I'd like to see these become constants, perhaps in an enum in a separate module. I'd add a TODO, or an issue perhaps, for all follow-ups?

clintandrewhall · 2018-12-18T20:36:59Z

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

@@ -137,6 +137,16 @@ const handleKeyDown = (commit, e, isEditable, remove) => {
  }
 };

+const handleKeyPress = (commit, e, isEditable) => {


The difference would be if you wanted to include control, shift, option, etc. Keypress would ignore keys combined with option on a mac, I believe.

clintandrewhall · 2018-12-18T20:37:30Z

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

+  const upcaseKey = key && key.toUpperCase();
+  if (isEditable && !isTextInput(target) && 'GU'.indexOf(upcaseKey) !== -1) {
+    commit('actionEvent', {
+      event: upcaseKey === 'G' ? 'group' : 'ungroup',


We need to get into the habit of creating constants for strings like this.

clintandrewhall · 2018-12-18T21:15:43Z

x-pack/plugins/canvas/public/state/actions/elements.js

+  ({ dispatch, getState }, rootElementIds, pageId) => {
+    const state = getState();
+
+    // todo consider doing the group membership collation in aeroelastic, or the Redux reducer, when adding templates


Consider matching a TODO with an issue number, even if it's a single issue that encompasses all of the follow-ups. This makes it easier to grep for all of the changes you want to make, (since searching for todo would probably yield far too many results)

clintandrewhall · 2018-12-18T21:17:06Z

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

    transformMatrix,
    a, // we currently specify half-width, half-height as it leads to
    b, // more regular math (like ellipsis radii rather than diameters)
  };
 };

+const shapeToElement = shape => {
+  return {
+    left: shape.transformMatrix[12] - shape.a,


Either a comment or a constant that defines to what 12 and 13 refer... if I were fixing a bug in this file, I'd be scared to death to touch these lines.

w33ble · 2018-12-18T21:23:06Z

When I create a new workpad, I see the following:

commons.bundle.js:166370 TypeError: Cannot read property 'groups' of undefined
    at getLocationFromIds (canvas.bundle.js:28265)
    at assignNodeProperties (canvas.bundle.js:28285)
...

UPDATE: I'm starting from the flights workpad that the sample data provides.

Load the flights workpad
Open the workpad modal and create a new workpad
See the above errors

elasticmachine · 2018-12-18T21:38:34Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

this might be caused by a race condition or something. this fix is probably just covering up some other bug :(

w33ble · 2018-12-18T21:48:32Z

@monfera I fixed the bug I was seeing, but I suspect the getLocationFromIds function actually being called with an invalid state because of some other issue.

Also, I notice that the state now tracks the size and position of the group. Here's a diff view when I resize a group of 2 elements:

It doesn't seem like that should exist in state, since it's a calculated value based on the size and position of the elements inside it.

w33ble · 2018-12-18T21:50:57Z

There also seems to be a floating point math issue on elements when you resize them as a group:

rashidkpc · 2018-12-18T22:12:42Z

Approved so long as Joe's comments above about the height and width parameters get taken care of before this ships.

elasticmachine · 2018-12-18T22:20:43Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

w33ble · 2018-12-18T22:48:18Z

I opened #27444 and #27446 to track the state issues.

w33ble

Since the open requests have their own issues now, and since this looks functionally sound, the PR LGTM.

w33ble · 2018-12-18T23:24:50Z

I'm going to merge this as-is. @monfera I'll leave it up to you to make sure the associated bugs are addressed before 6.6 ships.

* Canvas element grouping (squashed) fix: don't allow a tilted group feat: allow rotation as long as it's a multiple of 90 degrees same as previous but recursively minor refactor - predicate minor refactor - removed if minor refactor - extracted groupedShape minor refactor - extracted out magic; temporarily only enable 0 degree orientations minor refactor - recurse minor refactor - ignore annotations minor refactor - simplify recursion minor refactor - simplify recursion 2 removed key gestures remove ancestors 1 remove ancestors 2 remove ancestors 3 remove ancestors 4 * lint * separate elements and groups in storage * renamed `element...` to `node...` (except exported names and action payload props, for now) * be able to remove a group * fixing group deletion * not re-persisting unnecessarily * persist / unpersist group right on the keyboard action * solving inverted cascading for backward compatibility * fix failing test case * page cloning with group trees of arbitrary depth * extracted out cloneSubgraphs * basic copy-paste that handles a grouping tree of arbitrary depth * fix: when legacy dataset doesn't have `groups`, it should avoid an `undefined` * PR feedback: regularize group IDs * lint: curlies * schemaVersion bump * copy/paste: restore selection and 10px offset of newly pasted element * - fix regression with ad hoc groups - fix copy/paste offsetting * PR feedback: don't persist node `type` and group `expression` * chore: remove commented out code * chore: switch Object.entries to Object.keys * fix: handle undefined value this might be caused by a race condition or something. this fix is probably just covering up some other bug :(

w33ble · 2018-12-19T00:32:22Z

6.x 2a67310

monfera requested a review from rashidkpc November 19, 2018 08:56

monfera added the Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas label Nov 19, 2018

rashidkpc requested a review from alexfrancoeur November 19, 2018 22:47

monfera force-pushed the grouping-squashed branch from fc82399 to aad571a Compare November 21, 2018 18:04

cqliu1 mentioned this pull request Dec 3, 2018

Canvas clipboard #25543

Closed

2 tasks

monfera force-pushed the grouping-squashed branch from aad571a to ca66690 Compare December 14, 2018 00:23

monfera requested a review from a team as a code owner December 14, 2018 00:23

monfera changed the title ~~[Canvas][WIP] Persistent grouping~~ [Canvas][Layout Engine] Persistent grouping Dec 14, 2018

monfera self-assigned this Dec 14, 2018

monfera added the review label Dec 14, 2018

elastic deleted a comment from elasticmachine Dec 14, 2018

w33ble reviewed Dec 18, 2018

View reviewed changes

w33ble suggested changes Dec 18, 2018

View reviewed changes

w33ble added 2 commits December 18, 2018 14:02

chore: remove commented out code

20d575d

chore: switch Object.entries to Object.keys

6ee1beb

clintandrewhall reviewed Dec 18, 2018

View reviewed changes

fix: handle undefined value

ee0dad3

this might be caused by a race condition or something. this fix is probably just covering up some other bug :(

w33ble added v7.0.0 v6.6.0 labels Dec 18, 2018

rashidkpc approved these changes Dec 18, 2018

View reviewed changes

w33ble approved these changes Dec 18, 2018

View reviewed changes

w33ble mentioned this pull request Dec 18, 2018

[Grouping] Remove setTimeout when duplicating elements #27456

Closed

w33ble merged commit 1e7740a into elastic:master Dec 18, 2018

w33ble mentioned this pull request Dec 18, 2018

Backport [Canvas][Layout Engine] Persistent grouping #25854 #27459

Merged

[Canvas][Layout Engine] Persistent grouping #25854

[Canvas][Layout Engine] Persistent grouping #25854

Conversation

monfera commented Nov 19, 2018 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Nov 19, 2018

💚 Build Succeeded

monfera commented Nov 19, 2018 • edited Loading

elasticmachine commented Nov 19, 2018

monfera commented Nov 20, 2018 • edited Loading

w33ble commented Nov 20, 2018

rashidkpc commented Nov 20, 2018 • edited Loading

monfera commented Nov 20, 2018

elasticmachine commented Nov 21, 2018

💚 Build Succeeded

w33ble commented Nov 21, 2018

ryankeairns commented Nov 27, 2018 • edited Loading

monfera commented Nov 27, 2018

monfera commented Nov 27, 2018

monfera commented Dec 6, 2018 • edited Loading

alexfrancoeur commented Dec 6, 2018

monfera commented Dec 6, 2018

alexfrancoeur commented Dec 6, 2018 via email

monfera commented Dec 14, 2018

elasticmachine commented Dec 14, 2018

💚 Build Succeeded

w33ble Dec 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

w33ble Dec 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

w33ble left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

w33ble Dec 18, 2018 • edited Loading

Choose a reason for hiding this comment

w33ble Dec 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintandrewhall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

w33ble commented Dec 18, 2018 • edited Loading

elasticmachine commented Dec 18, 2018

💚 Build Succeeded

w33ble commented Dec 18, 2018

w33ble commented Dec 18, 2018

rashidkpc commented Dec 18, 2018

elasticmachine commented Dec 18, 2018

💚 Build Succeeded

w33ble commented Dec 18, 2018

w33ble left a comment

Choose a reason for hiding this comment

w33ble commented Dec 18, 2018

monfera commented Nov 19, 2018 •

edited

Loading

monfera commented Nov 19, 2018 •

edited

Loading

monfera commented Nov 20, 2018 •

edited

Loading

rashidkpc commented Nov 20, 2018 •

edited

Loading

ryankeairns commented Nov 27, 2018 •

edited

Loading

monfera commented Dec 6, 2018 •

edited

Loading

w33ble Dec 18, 2018 •

edited

Loading

w33ble Dec 18, 2018 •

edited

Loading

w33ble Dec 18, 2018 •

edited

Loading

w33ble Dec 18, 2018 •

edited

Loading

w33ble commented Dec 18, 2018 •

edited

Loading