Drag aggregations to sort instead of having up/down arrows. by bevacqua · Pull Request #6566 · elastic/kibana

bevacqua · 2016-03-17T19:12:53Z

Implements #5563.

Added drag and drop replacing up/down
Added enable/disable
- Support for save $state persistance

rashidkpc · 2016-03-18T21:19:55Z

@bevacqua you might want to test this against the feature/design branch. The changes are pretty minimal, but you should probably check it.

spalger · 2016-03-18T23:08:51Z

src/plugins/kibana/public/visualize/editor/agg_group.html

Is there a reason for this change? In general we try to keep the angular attributes first, though it's not formalized in the style guide or even used in this file 🎈

Ah, I guess I'm just used to having them the other way around. FWIW the <div right above this line is consistent with the change I had made. But good to know

spalger · 2016-03-19T00:01:32Z

I like how succinct the changes here are, but I'd love to see it rely a bit less on direct dom access.

To illustrate my point, this is how I'd probably organize this:

create a directive for both the sortable container and sortable item
in container directive
- expose a controller to the items, with a linkAgg(agg, element) method
- instantiate dragula, wire up the events and such, use the controllers element by injecting $element
- expose the dragging variable on $scope, so that the template can hide the add button when dragging starts
in item directive
- require the container directive
- in the link function call linkAgg() with own agg and element
in linkAgg(agg, element)
- maps agg instances to elements (maybe vise versa?) using a WeakMap
in the handlers for drag/drop/etc.
- use the elementToAgg map to find the agg for related elements

spalger · 2016-03-19T00:02:29Z

src/plugins/kibana/public/visualize/editor/agg_group.js

Are you not a fan of making the whole container draggable? I tried removing this function to see what would happen and was pleasantly surprised how "sloppy" I could get with my mouse and still get the job done.

Well. Affordances. I'd be fine with that if and only if we could clearly convey that aggregations are draggable. Right now the button combined with the cursor when its hovered over makes that pretty clear.

spalger · 2016-03-19T00:12:48Z

Oh, also, reordering the aggs doesn't seem to dirty the aggregation state. Rather than editing the $scope.group you need to update the order of $scope.vis.aggs

rashidkpc · 2016-03-25T20:08:05Z

@bevacqua is this still in progress?

bevacqua · 2016-03-28T17:22:14Z

@rashidkpc Yup! Just got back from a couple of holidays last week, getting back up to speed! Same goes for #6543

spalger · 2016-03-28T23:02:15Z

src/plugins/kibana/public/visualize/editor/draggable_item.js

$scope.draggableItemCtrl === this

…ort aggregations. elastic#5563

…lastic#5563

bevacqua · 2016-03-30T18:45:19Z

jenkins, test it

spalger · 2016-03-30T19:52:52Z

jenkins, test it

spalger · 2016-04-05T14:35:33Z

Not sure why the tests are failing here.

epixa · 2016-05-12T17:11:29Z

jenkins, test it

bevacqua · 2016-05-12T19:10:10Z

@epixa Thanks to Lee's thorough help I was able to identify the culprit. Tests are now passing.

epixa · 2016-05-18T19:16:07Z

src/plugins/kibana/public/visualize/editor/__tests__/draggable.js

+      expect($scope.drake.canMove(item[0])).to.eql(false);
+    });
+
+    it('shouldn\'t be able to move [draggable-item] if it has a handle', function () {


In the future, just use string templates instead of escaping the single quote. Or should not.

Right. Bad habit!

Don't bother changing it. That was just a note for posterity's sake.

epixa · 2016-05-18T19:33:58Z

I only found one issue (in Firefox, if that matters). If you're dragging the bottom bucket or trying to drag a bucket below the bottom bucket, the "Add sub-buckets" button appears to piggy back on the dragging behavior. It doesn't actually end up moving, but the effect while you're dragging makes it look like it will. Worse, you actually need to drag the bucket below the button itself in order to move it to the end:

bevacqua · 2016-05-24T01:50:21Z

@epixa The issue you mentioned should be fixed in 431ba4c

epixa · 2016-05-27T21:19:32Z

LGTM

## Summary `eui@74.0.2` ⏩ `eui@75.0.0` ___ ## [`75.0.0`](https://github.com/elastic/eui/tree/v75.0.0) - `EuiFlyout`s now automatically shard all fixed `EuiHeader`s on the page. This means that interactions (mouse & keyboard) with items inside `EuiHeader`s when flyouts are open will no longer trigger focus fighting ([#6566](elastic/eui#6566)) - `EuiFlyout`s now read out detailed screen reader dialog instructions and hints on open ([#6566](elastic/eui#6566)) **Bug fixes** - Fixed `EuiSelectable` options with incorrect `aria-posinset` indices when rendered with group labels not at the start of the array ([#6571](elastic/eui#6571)) - Fixed a bug with `EuiSearchBar` where filters with `multiSelect: false` were not able to select a new option when an option was already selected ([#6577](elastic/eui#6577)) **Breaking changes** - Removed the ability to customize the `role` prop of `EuiFlyout`s. `EuiFlyout`s should always be dialog roles for screen reader consistency. ([#6566](elastic/eui#6566)) - Removed `closeButtonAriaLabel` prop from `EuiFlyout` - use `closeButtonProps['aria-label']` instead ([#6566](elastic/eui#6566)) --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

## Summary `eui@74.0.2` ⏩ `eui@75.0.0` ___ ## [`75.0.0`](https://github.com/elastic/eui/tree/v75.0.0) - `EuiFlyout`s now automatically shard all fixed `EuiHeader`s on the page. This means that interactions (mouse & keyboard) with items inside `EuiHeader`s when flyouts are open will no longer trigger focus fighting ([elastic#6566](elastic/eui#6566)) - `EuiFlyout`s now read out detailed screen reader dialog instructions and hints on open ([elastic#6566](elastic/eui#6566)) **Bug fixes** - Fixed `EuiSelectable` options with incorrect `aria-posinset` indices when rendered with group labels not at the start of the array ([elastic#6571](elastic/eui#6571)) - Fixed a bug with `EuiSearchBar` where filters with `multiSelect: false` were not able to select a new option when an option was already selected ([elastic#6577](elastic/eui#6577)) **Breaking changes** - Removed the ability to customize the `role` prop of `EuiFlyout`s. `EuiFlyout`s should always be dialog roles for screen reader consistency. ([elastic#6566](elastic/eui#6566)) - Removed `closeButtonAriaLabel` prop from `EuiFlyout` - use `closeButtonProps['aria-label']` instead ([elastic#6566](elastic/eui#6566)) --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

bevacqua added review v5.0.0 labels Mar 17, 2016

bevacqua assigned spalger Mar 17, 2016

spalger reviewed Mar 18, 2016
View reviewed changes

spalger reviewed Mar 19, 2016
View reviewed changes

spalger assigned bevacqua and unassigned spalger Mar 19, 2016

spalger reviewed Mar 28, 2016
View reviewed changes

src/plugins/kibana/public/visualize/editor/draggable_item.js Outdated

Copy link
Copy Markdown

Contributor

spalger Mar 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$scope.draggableItemCtrl === this

bevacqua assigned spalger and unassigned bevacqua Mar 28, 2016

[ui] Replace up/down buttons with a drag handle that allows user to s…

bdf321b

…ort aggregations. elastic#5563

bevacqua force-pushed the feature/sort-dimensions-dragging branch from cac4752 to bdf321b Compare March 29, 2016 16:22

[ui/feature] Added ability to enable or disable aggregations as per e…

40b3171

…lastic#5563

Merge branch 'master' into feature/sort-dimensions-dragging

64c642d

spalger added needs updates and removed review labels Apr 5, 2016

spalger assigned bevacqua and unassigned spalger Apr 5, 2016

[cleanup] Add linking function back.

13220cb

[test] Only move draggable items by their drag handles.

1f60505

bevacqua assigned epixa and unassigned bevacqua May 12, 2016

epixa added review and removed needs updates labels May 15, 2016

epixa reviewed May 18, 2016
View reviewed changes

epixa added needs updates and removed review labels May 18, 2016

epixa assigned bevacqua and unassigned epixa May 18, 2016

[ui] Hide 'Add sub-buckets' button while dragging an aggregate.

431ba4c

bevacqua assigned epixa and unassigned bevacqua May 20, 2016

epixa added review and removed needs updates labels May 21, 2016

bevacqua merged commit 1cf2979 into elastic:master May 27, 2016

bevacqua unassigned epixa May 27, 2016

bevacqua mentioned this pull request Jun 1, 2016

Add (enable/disable) dimensions in visualization #5563

Closed

epixa added the v5.0.0-alpha4 label Jun 17, 2016

epixa added v5.0.0 and removed v5.0.0 labels Jun 28, 2016

BigFunger mentioned this pull request Sep 22, 2016

Draggable rework #8442

Merged

Conversation

bevacqua commented Mar 17, 2016

Uh oh!

rashidkpc commented Mar 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spalger commented Mar 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spalger commented Mar 19, 2016

Uh oh!

rashidkpc commented Mar 25, 2016

Uh oh!

bevacqua commented Mar 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bevacqua commented Mar 30, 2016

Uh oh!

spalger commented Mar 30, 2016

Uh oh!

spalger commented Apr 5, 2016

Uh oh!

epixa commented May 12, 2016

Uh oh!

bevacqua commented May 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epixa commented May 18, 2016

Uh oh!

bevacqua commented May 24, 2016

Uh oh!

epixa commented May 27, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants