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.

Sign up for GitHub

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

Jump to bottom

[Maps] allow adding multiple layers #67544

Merged
merged 8 commits into from
May 28, 2020
Merged

[Maps] allow adding multiple layers #67544

merged 8 commits into from
May 28, 2020

Conversation

nreese
Copy link
Contributor

@nreese nreese commented May 27, 2020

Fixes #67505

This PR updates "add layer" to support wizards that return multiple layers. This is required for a wizard to created pew pew map

@nreese nreese added release_note:enhancement [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v8.0.0 v7.9.0 labels May 27, 2020
@nreese nreese requested a review from kindsun May 27, 2020 19:04
@nreese nreese requested a review from a team as a code owner May 27, 2020 19:04
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis (Team:Geo)

@nreese nreese requested a review from kindsun May 28, 2020 13:33
kindsun
kindsun reviewed May 28, 2020
View reviewed changes
x-pack/plugins/maps/public/connected_components/add_layer_panel/flyout_footer/index.ts
closeFlyout: () => dispatch<any>(clearTransientLayerStateAndCloseFlyout()),
closeFlyout: () => {
dispatch(updateFlyout(FLYOUT_STATE.NONE));
dispatch<any>(removePreviewLayers());
Copy link
Contributor

Choose a reason for hiding this comment

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

There are a few other places where we're closing the flyout but not removing preview layers. We might want to create an action called something like closeFlyoutAndClearTempLayers that calls updateFlyout(FLYOUT_STATE.NONE) and also clears all preview and selected layers so we don't miss any cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where are those places? I suspect that in those places, it is not possible to have preview layers. In the last couple of minors, we have updated the UI to avoid opening flyout when its already opened to avoid having to do weird clean-ups like this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect that in those places, it is not possible to have preview layers

That might be obvious now but may not be as obvious for devs that come after. It's difficult to know for sure which flyout close actions should coincide with what clean up, whereas I find it very easy to reason about:

An action that cancels or completes layer adding can always:

  • clean up any temp layers since temp layers are an add layer construct
  • close flyout

All that said, this doesn't need to hold up this PR. This might be more of an offline architectural discussion.

@nreese nreese requested a review from kindsun May 28, 2020 13:59
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

kindsun
kindsun approved these changes May 28, 2020
View reviewed changes
Copy link
Contributor

@kindsun kindsun left a comment

Choose a reason for hiding this comment

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

This is really nice! It also opens up the possibility (with some more work) of previewing and uploading multiple files to a single index. lgtm!

  • code review
  • tested locally in chrome

@nreese nreese merged commit 7118e75 into elastic:master May 28, 2020
nreese added a commit to nreese/kibana that referenced this pull request May 28, 2020
@nreese
[Maps] allow adding multiple layers (elastic#67544)
9d1e4d4 
* [Maps] allow adding multiple layers

* update RenderWizardArguments arguments

* fix toc_entry jest test

* fix tslint error

* cleanup

* remove __transientLayerId from store signature

* rename setSelectedLayerToFirstPreviewLayer

* revert changes to es_search_source/create_source_editor.js
@nreese nreese mentioned this pull request May 28, 2020
Merged
nreese added a commit that referenced this pull request May 29, 2020
@nreese @elasticmachine
[Maps] allow adding multiple layers (#67544) (#67706)
04745ae 
* [Maps] allow adding multiple layers

* update RenderWizardArguments arguments

* fix toc_entry jest test

* fix tslint error

* cleanup

* remove __transientLayerId from store signature

* rename setSelectedLayerToFirstPreviewLayer

* revert changes to es_search_source/create_source_editor.js

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@thomasneirynck thomasneirynck mentioned this pull request Jun 12, 2020
Closed
@thomasneirynck thomasneirynck added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:enhancement labels Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kindsun kindsun kindsun approved these changes

Assignees
No one assigned
Labels
[Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation release_note:skip Skip the PR/issue when compiling release notes v7.9.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update "add layers" to support adding multiple layers
5 participants
@nreese @elasticmachine @kibanamachine @kindsun @thomasneirynck