[Lens] Drag dimension to replace #75895

wylieconlon · 2020-08-25T16:14:05Z

This only works when dragging dimensions to a different groups, such as "X-axis" to "Break down by". Example:

As a side effect of the drag changes, the table configuration is now split into "Break down by" and "Metrics":

Still to do: Functional tests and Kibana docs update.

Part of #51739

Checklist

Delete any items that are not applicable to this PR.

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

elasticmachine · 2020-08-25T16:14:07Z

Pinging @elastic/kibana-app (Team:KibanaApp)

cchaos · 2020-08-25T22:23:40Z

Context: Reviewing solely the "Replace" functionality

Aside from some stylistic tweaks to the dragging item (hover & focus states), this behavior works pretty well. I think more than the strikethrough, the more evident thing we can do to show that this will be moving the config is to hide the original placement.

When they're hovering over a new spot, whether a pre-existing config or empty config, the known paradigm will be that this will take its spot, so whatever we do to the styling of the new placement isn't as necessary.

Then we also need to be sure to use the correct grabbing cursor too. I had originally thought we should use copy for when hovering over any place that will accept it, but I think we should reserve that for when we can actual duplicate.

wylieconlon · 2020-08-25T23:10:00Z

@cchaos Thanks for the feedback! I implemented a visibility: hidden style to the dragged item if it's a column, because it needs to maintain its position in the DOM but not be rendered. The other change you've asked for is more wide-reaching and will affect all drag & drop behavior in Lens, so I will not implement it as part of this PR. It sounds like I should expect your approval on the PR soon- if not, please be specific about what you're blocking approval on.

mbondyra · 2020-08-26T07:47:26Z

x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/_layer_panel.scss

+.lnsLayerPanel__dimension-replace {
+  text-decoration: line-through;
+}
+


To avoid the effect of 'x' button being slower than the rest when dragging, ~~you could add a custom className and overwrite transition: all 250ms ease-in setting coming from EUI, eg: transition: all 250ms ease-out, visibility 0ms;~~
I found a better solution - can you instead animate opacity:0 and not visibility on .lnsLayerPanel__dimension-hidden? Opacity is more performant and solves the problem with the button.

Here's what I mean:

x-pack/plugins/lens/public/datatable_visualization/visualization.tsx

mbondyra · 2020-08-26T08:24:35Z

x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/_layer_panel.scss

@@ -27,6 +27,14 @@
  overflow: hidden;
 }

+.lnsLayerPanel__dimension-hidden {


nitpick: in other places in lens we use BEM notation with a format block__element--modifier so you might want to change it to .lnsLayerPanel__dimension--hidden and lnsLayerPanel__dimension--replace

BEM also indicates that you use a single dash for specific states. Like block__element-isDisabled. The single dash is technically correct here, but it's better to indicate state by adding the is in front like: lnsLayerPanel__dimension-isHidden

mbondyra

I left few small comments, but overall looks good 👍 I will leave my 'approve' when we're aligned with Caroline about the design details.

cchaos · 2020-08-26T15:28:10Z

Two more situations to think more about or make sure we follow up on.

1. Dragging a config that has no where to go

Right now you can start dragging a Y-axis config, but there's no place for it to go.

How should we handle this situation? I'm thinking that it's not right to completely disable the drag effect because it might look like a bug. Perhaps we need to not only indicate good drop zones but bad ones as well. Perhaps by just decreasing the opacity of the bad ones.

It may be mostly a temporary measure since we should eventually get to a place where any configs can be replaced, but it does at least set us up with something in the event we come across this situation that will happen.

2. Dragging between layers

wylieconlon · 2020-08-26T20:21:47Z

@cchaos I can't tell which comments of yours are meant to be blocking approval, and which are meant for further discussion. Both of your most recent comments need more discussion, and I'm not sure that they should be done in this PR. Here's why they need more discussion:

There are at least three approaches to adding a "not droppable" indicator:
- Currently all draggable items behave the same in Lens. We could have separate behavior for fields and operations, and start having diverging styling for those two.
- If an item can't be dropped anywhere, maybe we should prevent it from being dragged. This is more technically complex and would require a separate implementation.
- Finally, the simplest implementation which I prefer is to apply these styles to all drop targets in Lens. This is how it looks:

The GIF here is showing my preferred way of doing it. If you agree, I'll implement in this PR. If you disagree, let's implement it in a separate PR due to technical complexity.

Replacing operations between layers is a feature that I don't see a strong use case for. I can see it being useful for "copy" or "swap" behaviors. Can you explain why you want this?

cchaos · 2020-08-27T02:03:45Z

Finally, the simplest implementation which I prefer is to apply these styles to all drop targets in Lens. This is how it looks:

I agree with this approach and your implementation looks good to me. Lets do it 👍

Replacing operations between layers is a feature that I don't see a strong use case for. I can see it being useful for "copy" or "swap" behaviors. Can you explain why you want this?

Because "Replacing" doesn't just replace an existing config, but can move an X-axis from layer 1 to the newly created X-axis of of the newly created (empty) layer 2. And it seemed to me like replacing is a precursor to being able to swap and copy. If you disagree with this approach, that's fine.

wylieconlon · 2020-08-27T20:08:47Z

@cchaos I think this PR can be approved by you, if there is anything you're blocking approval on please be specific. I am not implementing dragging across layers in this PR. I'd welcome some clarification of the behavior you were expected in the parent issue. #51739

x-pack/plugins/lens/public/datatable_visualization/visualization.tsx

x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/_layer_panel.scss

wylieconlon · 2020-09-01T18:39:46Z

@cchaos I think the PR is approvable now. One of the changes you requested was already done, and the other is done now.

cchaos

Functionally this LGTM. 🎉

I'll end up doing some design clean up to the different DnD states once we get them hall worked out functionally.

I think @myasonik may have some a11y concerns that he may want to bring up proactively so we can address them early before it's all been configured.

myasonik · 2020-09-02T00:14:20Z

I took a look and this PR doesn't seem to make anything worse than what already existed so I'm actually not going to leave any a11y feedback right now

wylieconlon · 2020-09-03T21:11:13Z

@elasticmachine merge upstream

kibanamachine · 2020-09-03T23:42:24Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 35cf8fe

Build metrics

page load bundle size

id	value	diff	baseline
lens	873.9KB	+6.6KB	867.3KB

History

💚 Build #71672 succeeded c4333d5
💚 Build #71025 succeeded d501b8b
💚 Build #70690 succeeded 1b172f4
💚 Build #70410 succeeded 7830ca0
💚 Build #70355 succeeded 4567462

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

mbondyra · 2020-09-04T13:49:17Z

Tested on Firefox, looks good!

* [Lens] Drag to replace * Add jest tests for drag and drop * Fix bug in dragging to empty * Hide dragged dimension when drag starts * Make table metrics required * Update class names * Implement styles on non-droppable items * Replace drag and drop image in docs * Remove extra specificity Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>

* master: (47 commits) Do not require id & description when creating a logstash pipeline (elastic#76616) Remove commented src/core/tsconfig file (elastic#76792) Replaced whitelistedHosts with allowedHosts in actions ascii docs (elastic#76731) [Dashboard First] Genericize Attribute Service (elastic#76057) [ci-metrics] unify distributable file count metrics (elastic#76448) [Security Solution][Detections] Handle conflicts on alert status update (elastic#75492) [eslint] convert to @typescript-eslint/no-unused-expressions (elastic#76471) [DOCS] Add default time range filter to advanced settings (elastic#76414) [Security Solution] Refactor NetworkTopNFlow to use Search Strategy (elastic#76249) [Dashboard] Update Index Patterns when Child Index Patterns Change (elastic#76356) [ML] Add option to Advanced Settings to set default time range filter for AD jobs (elastic#76347) Add CSM app to CODEOWNERS (elastic#76793) [Security Solution][Exceptions] - Updates exception item find sort field (elastic#76685) [Security Solution][Detections][Tech Debt] - Move to using common io-ts types (elastic#75009) [Lens] Drag dimension to replace (elastic#75895) URI encode the index names we fetch in the fetchIndices lib function. (elastic#76584) [Security Solution] Resolver retrieve entity id of documents without field mapped (elastic#76562) [Ingest Manager] validate agent route using AJV instead kbn-config-schema (elastic#76546) Updated non-dev usages of node-forge (elastic#76699) [Ingest Pipelines] Processor forms for processors K-S (elastic#75638) ...

wylieconlon added 3 commits August 20, 2020 17:57

[Lens] Drag to replace

babc6d4

Merge remote-tracking branch 'origin/master' into lens/drag-to-replace

f95c080

Add jest tests for drag and drop

ce570be

wylieconlon added Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 Feature:Lens v7.10.0 labels Aug 25, 2020

wylieconlon requested review from cchaos, dej611, mbondyra and a team August 25, 2020 16:14

wylieconlon requested a review from a team as a code owner August 25, 2020 16:14

wylieconlon added the release_note:enhancement label Aug 25, 2020

wylieconlon added 2 commits August 25, 2020 14:20

Fix bug in dragging to empty

aaa59f5

Merge remote-tracking branch 'origin/master' into lens/drag-to-replace

4567462

wylieconlon requested a review from AlonaNadler August 25, 2020 18:39

wylieconlon added 2 commits August 25, 2020 18:53

Merge remote-tracking branch 'origin/master' into lens/drag-to-replace

79feebb

Hide dragged dimension when drag starts

7830ca0

wylieconlon mentioned this pull request Aug 25, 2020

[Lens] Add additional drag and drop states including "partially allowed" state #75938

Closed

mbondyra reviewed Aug 26, 2020

View reviewed changes

x-pack/plugins/lens/public/datatable_visualization/visualization.tsx Show resolved Hide resolved

mbondyra reviewed Aug 26, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into lens/drag-to-replace

4899066

Make table metrics required

023a481

Update class names

1b172f4

wylieconlon added 3 commits August 27, 2020 12:23

Merge remote-tracking branch 'origin/master' into lens/drag-to-replace

45978e8

Implement styles on non-droppable items

01c6069

Replace drag and drop image in docs

d501b8b

cchaos suggested changes Aug 31, 2020

View reviewed changes

x-pack/plugins/lens/public/datatable_visualization/visualization.tsx Show resolved Hide resolved

x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/_layer_panel.scss Show resolved Hide resolved

wylieconlon added 2 commits September 1, 2020 14:29

Merge remote-tracking branch 'origin/master' into lens/drag-to-replace

f6e9c23

Remove extra specificity

c4333d5

wylieconlon requested a review from cchaos September 1, 2020 18:38

cchaos approved these changes Sep 1, 2020

View reviewed changes

wylieconlon requested a review from mbondyra September 2, 2020 18:39

Merge branch 'master' into lens/drag-to-replace

35cf8fe

mbondyra approved these changes Sep 4, 2020

View reviewed changes

wylieconlon merged commit 9ca8551 into elastic:master Sep 4, 2020

wylieconlon deleted the lens/drag-to-replace branch September 4, 2020 14:11

wylieconlon mentioned this pull request Sep 4, 2020

[7.x] [Lens] Drag dimension to replace (#75895) #76776

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] Drag dimension to replace #75895

[Lens] Drag dimension to replace #75895

wylieconlon commented Aug 25, 2020 •

edited

Loading

elasticmachine commented Aug 25, 2020

cchaos commented Aug 25, 2020

wylieconlon commented Aug 25, 2020

mbondyra Aug 26, 2020 •

edited

Loading

mbondyra Aug 26, 2020

cchaos Aug 26, 2020

mbondyra left a comment

cchaos commented Aug 26, 2020

wylieconlon commented Aug 26, 2020

cchaos commented Aug 27, 2020

wylieconlon commented Aug 27, 2020

wylieconlon commented Sep 1, 2020

cchaos left a comment

myasonik commented Sep 2, 2020

wylieconlon commented Sep 3, 2020

kibanamachine commented Sep 3, 2020

mbondyra commented Sep 4, 2020 •

edited

Loading

[Lens] Drag dimension to replace #75895

[Lens] Drag dimension to replace #75895

Conversation

wylieconlon commented Aug 25, 2020 • edited Loading

Checklist

elasticmachine commented Aug 25, 2020

cchaos commented Aug 25, 2020

wylieconlon commented Aug 25, 2020

mbondyra Aug 26, 2020 • edited Loading

Choose a reason for hiding this comment

mbondyra Aug 26, 2020

Choose a reason for hiding this comment

cchaos Aug 26, 2020

Choose a reason for hiding this comment

mbondyra left a comment

Choose a reason for hiding this comment

cchaos commented Aug 26, 2020

1. Dragging a config that has no where to go

2. Dragging between layers

wylieconlon commented Aug 26, 2020

cchaos commented Aug 27, 2020

wylieconlon commented Aug 27, 2020

wylieconlon commented Sep 1, 2020

cchaos left a comment

Choose a reason for hiding this comment

myasonik commented Sep 2, 2020

wylieconlon commented Sep 3, 2020

kibanamachine commented Sep 3, 2020

💚 Build Succeeded

Build metrics

page load bundle size

History

mbondyra commented Sep 4, 2020 • edited Loading

wylieconlon commented Aug 25, 2020 •

edited

Loading

mbondyra Aug 26, 2020 •

edited

Loading

mbondyra commented Sep 4, 2020 •

edited

Loading