[Canvas] Alignment and distribution by monfera · Pull Request #39132 · elastic/kibana

monfera · 2019-06-17T21:17:28Z

Summary

Implements alignment and distribution.

Alignment:

Distribution, then some alignments:

Checklist

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

This was checked for cross-browser compatibility, including a check 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 · 2019-06-17T21:17:31Z

Pinging @elastic/kibana-canvas

elasticmachine · 2019-06-17T22:22:08Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-19T16:25:54Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-19T17:22:41Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

ryankeairns

@monfera first off, this is fantastic, thank you!

The good news: there is a rather common set of icons we can use for these!

The not-so-good news: I'm pretty backlogged at the moment and will need some time to get them done. If you'd like to get this merged, I can come back through add them once they're created. In the meantime, you can simply remove the icons for now.

I also have one small request that is quite subjective :) , can you remove the top borders on the Align elements and Distribute elements items so that we end up with this:

These will be very well-received features.

ryankeairns · 2019-06-20T23:01:54Z

Working on adding these icons to EUI in time for Kibana 7.3: elastic/eui#2070

monfera · 2019-06-21T07:40:12Z

Thanks @ryankeairns! Will they import like the other icons over there, or do I need to do something special, or are you planning to add a commit?

ryankeairns · 2019-06-21T13:01:04Z

Thanks @ryankeairns! Will they import like the other icons over there, or do I need to do something special, or are you planning to add a commit?

Upon updating from Master, you would be able to simply change the icon type (i.e. name).

monfera · 2019-06-21T21:47:25Z

Those things are awesome, Ryan, thanks! I see it's merged into eui but kibana master apparently hasn't picked it up yet, if you know when it happens, can you tap me on the shoulder? Or should I bump it myself in this PR and I don't do damage with it? Btw. should editorItemRight in your PR be editorItemAlignRight?

Lines now removed!

elasticmachine · 2019-06-21T22:52:30Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

ryankeairns · 2019-06-24T13:44:15Z

@monfera Greg is working on a new release and will then update Kibana. If your PR is ready prior to that happening, you can merge yours and I will add the icons once Kibana has been updated.

It looks like editorItemRight is only in my CHAGELOG comment 😌 ... I'll get that fixed in my next PR!

monfera · 2019-06-24T15:44:09Z

Thanks @ryankeairns - I should've looked further in the code indeed, it's just in the comment (this PR uses the compliant name)

ryankeairns · 2019-06-25T13:58:08Z

The design (icon) changes look good, just waiting on the EUI update in Kibana to come through.

elasticmachine · 2019-06-25T14:58:27Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

cqliu1

This is such a neat feature! It makes lining up multiple elements so much easier. I just had a couple comments, but the rest looks good.

cqliu1 · 2019-06-25T23:29:25Z

x-pack/legacy/plugins/canvas/public/lib/element_handler_creators.ts

  },
 };

+// handlers for group and ungroup


Nit: Can you update this comment?

cqliu1 · 2019-06-25T23:29:40Z

x-pack/legacy/plugins/canvas/public/components/sidebar_header/sidebar_header.tsx

+    };
+
+    this._getAlignmentMenuItems(close).forEach(menuFiller);
+    this._getDistributionMenuItems(close).forEach(menuFiller);


Nit: You implemented this differently than I did with _getLayerMenuItems. Why do you return the object wrapped in an array here instead of just the MenuTuple object?

Since _getLayerMenuItems, _getAlignmentMenuItems, and _getDistributionMenuItems are all similar functions, they should be implemented the same way, so it'd be great if you could refactor _getLayerMenuItems like the two new functions or vice versa.

Also, would it make sense to take the conditional check for the number of selectedNodes out of the individual functions and wrap the condition around these two function calls?

Here's an example of how I imagine this code looking (with all of the functions returning a MenuTuple object)

Suggested change

this._getDistributionMenuItems(close).forEach(menuFiller);

const menuFiller = ({ menuItem, panel }: MenuTuple) => {

items.push(menuItem);

panels.push(panel);

};

if (showLayerControls) {

menuFiller(this._getLayerMenuItems();

}

if(selectedNodes.length > 2) {

menuFiller(this._getAlignmentMenuItems(close));

menuFiller(this._getDistributionMenuItems(close));

}

Or alternatively (with all of the functions returning a MenuTuple[] array):

Suggested change

this._getDistributionMenuItems(close).forEach(menuFiller);

const menuFiller = ({ menuItem, panel }: MenuTuple) => {

items.push(menuItem);

panels.push(panel);

};

if (showLayerControls) {

this._getLayerMenuItems(close).forEach(menuFiller);

}

if(selectedNodes.length > 2) {

this._getAlignmentMenuItems(close).forEach(menuFiller);

this._getDistributionMenuItems(close).forEach(menuFiller);

}

@cqliu1 thanks for the feedback, looking at it with a fresh pair of eyes, uniformity and just plain simplicity supports your take, now pushed

cqliu1 · 2019-06-25T23:48:47Z

@monfera Were you planning to add shortcuts for these actions?

monfera · 2019-06-26T10:00:29Z

@cqliu1 I haven't seen shortcuts required and wasn't surprised as similar software often also doesn't have shortcuts for these. While technically possible, the concern is that there's an overload of shortcuts for a comparatively rare operation: 3 ops: align horizoontally, align vertically and distribute; 2 or 3 suboptions each. Either resulting in somewhat random keys or they somehow involve arrow keys and modifiers.

Having said it, we could get the discussion going on shortcuts, can be a follow-up PR if it doesn't fit the time frame.

Also it's a prime candidate for an eventual context menu

ryankeairns · 2019-06-26T17:37:05Z

I agree with @monfera 's assessment. These operations don't typically have shortcuts (at least not that I have experienced) and we could always add them later if the case arises. A context menu seems like a good solution, with regards to discoverability, in the long run.

ryankeairns · 2019-06-26T18:49:57Z

Kibana has been updated to EUI 12.1.0. Please rebase to see the new icons. Thanks!

elasticmachine · 2019-06-26T21:04:22Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

cqliu1

LGTM 👍

ryankeairns

👍 LGTM, well done, great addition!

[Canvas] Alignment and distribution

elasticmachine · 2019-08-06T20:13:47Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

monfera added release_note:enhancement WIP Work in progress enhancement New value added to drive a business result Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// v7.3.0 labels Jun 17, 2019

monfera requested a review from a team as a code owner June 17, 2019 21:17

monfera self-assigned this Jun 17, 2019

monfera requested a review from cqliu1 June 18, 2019 17:05

monfera force-pushed the alignment-distribution branch 3 times, most recently from 8c2e4e5 to ffd0a10 Compare June 19, 2019 16:14

elastic deleted a comment from elasticmachine Jun 19, 2019

ryankeairns reviewed Jun 19, 2019

View reviewed changes

ryankeairns mentioned this pull request Jun 20, 2019

Add alignment and distribution icons elastic/eui#2067

Closed

monfera force-pushed the alignment-distribution branch from ffd0a10 to 72e9fc2 Compare June 21, 2019 21:49

monfera changed the title ~~[Canvas][WIP] Alignment and distribution~~ [Canvas] Alignment and distribution Jun 21, 2019

monfera added review and removed WIP Work in progress labels Jun 21, 2019

monfera force-pushed the alignment-distribution branch from 72e9fc2 to 881a524 Compare June 25, 2019 08:46

cqliu1 added the v8.0.0 label Jun 25, 2019

cqliu1 requested changes Jun 25, 2019

View reviewed changes

monfera added 7 commits June 26, 2019 21:24

Affordances

02e2b4d

Verbified action names (thanks @cqliu1)

aff6aa5

Alignments work

909597d

Distributions

9d61eae

Removed lines; updated to new EUI icons; rebased

9f02828

PR feedback

11fa5a7

PR feedback 2

de67169

monfera force-pushed the alignment-distribution branch from 34d1f13 to de67169 Compare June 26, 2019 19:25

PR feedback 3 - extract more out of fillMenu

a685432

elastic deleted a comment from elasticmachine Jun 26, 2019

monfera requested a review from cqliu1 June 26, 2019 20:16

cqliu1 approved these changes Jun 26, 2019

View reviewed changes

ryankeairns self-requested a review June 27, 2019 01:42

ryankeairns approved these changes Jun 27, 2019

View reviewed changes

monfera merged commit 5fb13a8 into master Jun 27, 2019

monfera deleted the alignment-distribution branch June 27, 2019 09:34

monfera mentioned this pull request Jun 27, 2019

[7.x] [Canvas] Alignment and distribution (#39132) #39779

Merged

monfera added a commit that referenced this pull request Jun 27, 2019

[Canvas] Alignment and distribution (#39132) (#39779)

6d903c0

[Canvas] Alignment and distribution

monfera added a commit to monfera/kibana that referenced this pull request Jun 27, 2019

[Canvas] Alignment and distribution (elastic#39132)

19dd738

[Canvas] Alignment and distribution

kibanamachine mentioned this pull request Jul 12, 2019

[meta] New Layout Engine #23040

Closed

16 tasks

-    this._getDistributionMenuItems(close).forEach(menuFiller);
+    const menuFiller = ({ menuItem, panel }: MenuTuple) => {
+      items.push(menuItem);
+      panels.push(panel);
+    };
+    if (showLayerControls) {
+      menuFiller(this._getLayerMenuItems();
+    }
+    if(selectedNodes.length > 2) {
+      menuFiller(this._getAlignmentMenuItems(close));
+      menuFiller(this._getDistributionMenuItems(close));
+    }

Conversation

monfera commented Jun 17, 2019 • edited by KOTungseth Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Jun 17, 2019

Uh oh!

elasticmachine commented Jun 17, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Jun 19, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Jun 19, 2019

💚 Build Succeeded

Uh oh!

ryankeairns left a comment

Choose a reason for hiding this comment

Uh oh!

ryankeairns commented Jun 20, 2019

Uh oh!

monfera commented Jun 21, 2019

Uh oh!

ryankeairns commented Jun 21, 2019

Uh oh!

monfera commented Jun 21, 2019

Uh oh!

elasticmachine commented Jun 21, 2019

💚 Build Succeeded

Uh oh!

ryankeairns commented Jun 24, 2019

Uh oh!

monfera commented Jun 24, 2019

Uh oh!

ryankeairns commented Jun 25, 2019

Uh oh!

elasticmachine commented Jun 25, 2019

💚 Build Succeeded

Uh oh!

cqliu1 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cqliu1 Jun 25, 2019

Choose a reason for hiding this comment

Uh oh!

cqliu1 Jun 25, 2019

Choose a reason for hiding this comment

Uh oh!

monfera Jun 26, 2019

Choose a reason for hiding this comment

Uh oh!

cqliu1 commented Jun 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

monfera commented Jun 26, 2019

Uh oh!

ryankeairns commented Jun 26, 2019

Uh oh!

ryankeairns commented Jun 26, 2019

Uh oh!

elasticmachine commented Jun 26, 2019

💚 Build Succeeded

Uh oh!

cqliu1 left a comment

Choose a reason for hiding this comment

Uh oh!

ryankeairns left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Aug 6, 2019

💔 Build Failed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

monfera commented Jun 17, 2019 •

edited by KOTungseth

Loading

cqliu1 left a comment •

edited

Loading

cqliu1 commented Jun 25, 2019 •

edited

Loading