[Lens] Implement drag and drop between layers #132018

mbondyra · 2022-05-11T12:06:26Z

Summary

Enhances drag and drop to be able to move between layers.
It's not as big as it seems, most of it is tests and refactoring.

demo.mov

Refactoring

Simplifying arguments passed to getDropProps and onDrop - the params were consolidated to source, target and the others.
Reusing some helper functions between noDatasource and datasource-involving onDrop, eg. isOperationFromCompatibleGroup

Points of tests - I paid attention to those edgecases, let me know if I might have skipped something

Formulas all drop options
Moving average option
Top values sorted by value field (referencing another column)
When having two dimensions with non auto axis side and moving one to another column, I don't move this setting (so the column gets auto axis side)
A11y
a. How focus is placed (a11y)
b. Announcements

elasticmachine · 2022-06-18T14:19:42Z

Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors)

drewdaemon

Wow @mbondyra , this looks like the culmination of a huge effort! Everything I tested works great. It's fun to have a tour of code I haven't seen before.

Ran out of time today, so I'm leaving a partial review which I can pick back up on Monday.

x-pack/test/functional/apps/lens/group3/reference_lines.ts

x-pack/plugins/lens/public/xy_visualization/visualization_helpers.tsx

drewdaemon · 2022-06-18T16:35:09Z

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

+    if (!targetLayer) {
+      return props.prevState;
+    }


When does this condition occur? I can't think of a context within the UI where a drop occurs outside the context of a layer...

So I am actually not sure how to tackle these kind of problems nicely.

I refer to the problems when

we pass some kind of id of object to the function

We then look up the object with this id in an object or find it an array

We're sure that the object exist, but typescript will complain...

So we either have to force it with ! or make a check if (obj[id]) return or throw an error.

Sometimes it happens in Lens that something is implemented in desynchronized way (for example switching layers) and then actually the impossible happens so I got a habit of checking if something exists even if I am sure it is so at least the editor doesn't break for the user 😅 . I would be happy to have a conversation about how to tackle these cases smarter 🙂

You've articulated the dilemma well. Thinking out loud here.

When we make code robust to situations that should never occur it sometimes creates harder-to-find bugs than if we just let the code obviously crash and burn.

On the other hand, as a user, I don't want a minor (possibly inconsistent) bug to become so disruptive that the application is no longer useful (or worse, that I lose my work).

Thinking about this drag-and-drop case, I agree that it seems like the handling should be bullet-proof because, like you say, the impossible could happen and the best thing for the user is probably to shove it under the rug.

If I'm dealing with a different instance of the same pattern you described I might feel differently. For example, if a visualization I'm expecting to be in the visualizationMap isn't present, that's a big enough issue that I think the best thing for the user is to make the problem obvious so that we catch it early before the release.

And maybe our release cadence and development process inform these decisions, too. For example, maybe obvious bugs become more attractive with a slow release schedule because we have a long time to catch things before actually putting software in front of our users.

Someone must have written a book about this. It could be a good discussion to have as a team.

But, I think how you're doing it in this case makes sense. 👍

x-pack/plugins/lens/public/xy_visualization/visualization.test.ts

x-pack/plugins/lens/public/indexpattern_datasource/operations/time_scale_utils.ts

x-pack/plugins/lens/public/indexpattern_datasource/operations/layer_helpers.ts

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/index.ts

x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/droppable/on_drop_handler.ts

...lugins/lens/public/indexpattern_datasource/dimension_panel/droppable/on_drop_handler.test.ts

drewdaemon

Looked through everything now. No further comments on my part 👍

mbondyra · 2022-06-22T09:18:46Z

@elasticmachine merge upstream

kibana-ci · 2022-06-23T07:00:09Z

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`lens`	517	524	+7

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`lens`	1.2MB	1.2MB	+7.4KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`lens`	35	39	+4

Unknown metric groups

API count

id	before	after	diff
`lens`	594	602	+8

References to deprecated APIs

id	before	after	diff
`lens`	14	13	-1

History

💚 Build #52325 succeeded 6668c3e
💚 Build #52030 succeeded 0f79ee0
💚 Build #51730 succeeded 664f6d4
💚 Build #51636 succeeded 661f036
💔 Build #51613 failed 46bcda1
💔 Build #51591 failed e7c69ba

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

stratoula

This works fine Marta! I cant find any regression, great enhancement 👏

mbondyra force-pushed the lens/dragDropBetweenLayers branch 5 times, most recently from d5f8bb1 to acb4737 Compare May 26, 2022 14:04

mbondyra force-pushed the lens/dragDropBetweenLayers branch 5 times, most recently from 914fb19 to 94a9009 Compare June 1, 2022 18:18

mbondyra changed the title ~~[WIP] drag and drop between layers~~ [Lens] Implement drag and drop between layers Jun 1, 2022

mbondyra added release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens v8.4.0 labels Jun 1, 2022

mbondyra force-pushed the lens/dragDropBetweenLayers branch 4 times, most recently from e594ff0 to a60a707 Compare June 7, 2022 14:02

mbondyra force-pushed the lens/dragDropBetweenLayers branch 11 times, most recently from f2ffe72 to bf4644a Compare June 17, 2022 09:01

mbondyra force-pushed the lens/dragDropBetweenLayers branch 4 times, most recently from 46bcda1 to 661f036 Compare June 17, 2022 18:57

mbondyra marked this pull request as ready for review June 18, 2022 14:19

mbondyra requested review from a team as code owners June 18, 2022 14:19

drewdaemon reviewed Jun 19, 2022

View reviewed changes

mbondyra force-pushed the lens/dragDropBetweenLayers branch 2 times, most recently from 49fb2d6 to 39b2ea4 Compare June 20, 2022 08:26

added dnd between layers

664f6d4

mbondyra force-pushed the lens/dragDropBetweenLayers branch from 39b2ea4 to 664f6d4 Compare June 20, 2022 08:27

drewdaemon reviewed Jun 20, 2022

View reviewed changes

mbondyra removed the request for review from a team June 20, 2022 17:35

Andrew's comments addressed

0f79ee0

drewdaemon approved these changes Jun 21, 2022

View reviewed changes

kibanamachine and others added 2 commits June 22, 2022 05:18

Merge branch 'main' into lens/dragDropBetweenLayers

6668c3e

Merge branch 'main' into lens/dragDropBetweenLayers

c782ab8

stratoula approved these changes Jun 23, 2022

View reviewed changes

mbondyra merged commit d11c0be into elastic:main Jun 23, 2022

kibanamachine added the backport:skip This commit does not require backporting label Jun 23, 2022

mbondyra deleted the lens/dragDropBetweenLayers branch June 23, 2022 07:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] Implement drag and drop between layers #132018

[Lens] Implement drag and drop between layers #132018

mbondyra commented May 11, 2022 •

edited

Loading

elasticmachine commented Jun 18, 2022

drewdaemon left a comment

drewdaemon Jun 18, 2022

mbondyra Jun 21, 2022

drewdaemon Jun 21, 2022

drewdaemon left a comment

mbondyra commented Jun 22, 2022

kibana-ci commented Jun 23, 2022

API count

References to deprecated APIs

stratoula left a comment

[Lens] Implement drag and drop between layers #132018

[Lens] Implement drag and drop between layers #132018

Conversation

mbondyra commented May 11, 2022 • edited Loading

Summary

Refactoring

Points of tests - I paid attention to those edgecases, let me know if I might have skipped something

elasticmachine commented Jun 18, 2022

drewdaemon left a comment

Choose a reason for hiding this comment

drewdaemon Jun 18, 2022

Choose a reason for hiding this comment

mbondyra Jun 21, 2022

Choose a reason for hiding this comment

drewdaemon Jun 21, 2022

Choose a reason for hiding this comment

drewdaemon left a comment

Choose a reason for hiding this comment

mbondyra commented Jun 22, 2022

kibana-ci commented Jun 23, 2022

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Async chunks

Public APIs missing exports

API count

References to deprecated APIs

History

stratoula left a comment

Choose a reason for hiding this comment

mbondyra commented May 11, 2022 •

edited

Loading