Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Lens] Duplicate layers #140603

Merged
merged 19 commits into from
Sep 20, 2022
Merged

[Lens] Duplicate layers #140603

merged 19 commits into from
Sep 20, 2022

Conversation

alexwizp
Copy link
Contributor

@alexwizp alexwizp commented Sep 13, 2022

Closes: #132140

Describe the feature:
Added the ability to clone Data / Annotations / Reference line layers

What was changed:

  • LayerButtons component was added. If we have more than 2 actions, then a Сontext Menu appears;
  • remove_layer_button.tsx was refactored to work from Context Menu;
  • x-pack/plugins/lens/public/utils.ts added new generic method (renewIDs) to renew ids';
  • some JEST tests were added.

Notes:
For visualizations that do not support working with layers, the design of the button has not changed.

image

Screens:

  1. CloningData Layers
Screen.Recording.2022-09-16.at.18.00.37.mov
  1. Cloning 'Annotations'
Screen.Recording.2022-09-16.at.18.02.51.mov
  1. Cloning 'Reference Lines'
Screen.Recording.2022-09-16.at.18.04.03.mov

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@elastic elastic deleted a comment from kibana-ci Sep 16, 2022
@@ -89,8 +89,6 @@ export type IndexPatternField = FieldSpec & {
runtime?: boolean;
};

export type ErrorCallback = (e: { message: string }) => void;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not related to that PR, just a removing unused code

@alexwizp alexwizp self-assigned this Sep 16, 2022
@alexwizp alexwizp added v8.5.0 Team:Visualizations Visualization editors, elastic-charts and infrastructure release_note:feature Makes this part of the condensed release notes Feature:Lens labels Sep 16, 2022
@alexwizp alexwizp added the backport:skip This commit does not require backporting label Sep 16, 2022
@alexwizp alexwizp requested a review from mbondyra September 16, 2022 19:52
@alexwizp alexwizp changed the title [WIP][Lens] Duplicate layers [Lens] Duplicate layers Sep 19, 2022
@alexwizp alexwizp marked this pull request as ready for review September 19, 2022 08:55
@alexwizp alexwizp requested a review from a team as a code owner September 19, 2022 08:55
@elasticmachine
Copy link
Contributor

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

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

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

Very nicely written code and I couldn't find any problems with the solution 👏🏼
I think recursively updating ids is fine and works well.

One nit UX thing is that maybe we should not allow to clone an empty layer? So when it's empty, let's not show the option or disable it. Do you think it'd be easy to add?

@alexwizp
Copy link
Contributor Author

One nit UX thing is that maybe we should not allow to clone an empty layer? So when it's empty, let's not show the option or disable it. Do you think it'd be easy to add?

It's probably not hard to do, I just don't like that in this case, some layers will have a context menu and some only delete button(if they are empty) + there will be separate logic for annotations.
I vote to leave it as it is, @MichaelMarcialis , could you take a look?

alexwizp and others added 2 commits September 19, 2022 16:10
# Conflicts:
#	x-pack/plugins/lens/public/visualizations/xy/visualization.tsx
@drewdaemon
Copy link
Contributor

Really nice work. I will be using the layer context menu when I address #8143, so it's nice to see it being added!

WDYT about renaming the following files?

  • layer_buttons -> layer_actions
  • clone_layer_button.tsx -> clone_layer_action.tsx
  • remove_layer_button.tsx -> remove_layer_action.tsx

Copy link
Contributor

@MichaelMarcialis MichaelMarcialis left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together, @alexwizp. This looks lovely! I'm leaving you a few comments below for your review, but nothing worth holding you up over. Assuming you can address them, I'm approving now.

  • Similar to my code comment below, can we also remove the index number that is currently being appended to the Delete layer and Reset layer actions?

  • Can the content menu text for Delete layer and Reset layer be changed to red to match the icon?

  • Can we change all Reset layer text to Clear layer?

  • Regarding whether or not we should hide or disable the duplicate action when a layer has no dimensions populated, I think it's fine to leave it as it is currently (i.e. allow users to duplicate empty layers). It essentially just ends up functioning as if the user created a new visualization layer, so I don't see it harming the user experience to allow it.

alexwizp and others added 2 commits September 19, 2022 18:15
…onfig_panel/layer_buttons/clone_layer_button.tsx

Co-authored-by: Michael Marcialis <[email protected]>
…onfig_panel/layer_buttons/layer_buttons.tsx

Co-authored-by: Michael Marcialis <[email protected]>
@alexwizp
Copy link
Contributor Author

Thank you @andrewctate @MichaelMarcialis. Will apply your comments tomorrow morning

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lens 906 909 +3

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 553 557 +4

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 +3.9KB
Unknown metric groups

API count

id before after diff
lens 640 645 +5

History

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

cc @alexwizp

@alexwizp alexwizp merged commit 260ea9a into elastic:main Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Lens release_note:feature Makes this part of the condensed release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] Should allow a copy from visualisation layer
7 participants