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

[Maps] allow saving maps to dashboards #88759

Merged
merged 7 commits into from
Jan 26, 2021
Merged

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Jan 19, 2021

This PR adds support for SavedObjectSaveModalDashboard so when users save a new map they are presented with the option to save the map directly to a dashboard.

Screen Shot 2021-01-19 at 2 06 59 PM

@elasticmachine
Copy link
Contributor

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

@nreese nreese requested review from a team as code owners January 20, 2021 14:46
Copy link
Member

@afharo afharo left a comment

Choose a reason for hiding this comment

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

Core changes (x-pack/test/saved_object_tagging/functional/tests/maps_integration.ts) LGTM!

@kindsun
Copy link
Contributor

kindsun commented Jan 21, 2021

Nice PR! Seems to work well overall. A few flow-related questions. Not sure how much control we have over these pieces but I'll still float them:

  • Why is the only option for saving on the dashboard option Save and go to Dashboard? Shouldn't a user be able to save it to a dashboard without redirecting? It's a pretty normal workflow to save your map periodically as you construct it without wanting to navigate away from the Maps app. If you save it to your library, it appears you can't save it to a dashboard after that which makes it feel kind of "all or nothing" on the first save.
  • Can we change the default in the pop-up to No dasboard, but add to library?
  • Related to the last question, should we minimally change the default radio button selected depending on whether a user has any existing dashboards?

@nreese
Copy link
Contributor Author

nreese commented Jan 21, 2021

@elasticmachine merge upstream

@nreese
Copy link
Contributor Author

nreese commented Jan 21, 2021

Why is the only option for saving on the dashboard option Save and go to Dashboard? Shouldn't a user be able to save it to a dashboard without redirecting? It's a pretty normal workflow to save your map periodically as you construct it without wanting to navigate away from the Maps app. If you save it to your library, it appears you can't save it to a dashboard after that which makes it feel kind of "all or nothing" on the first save.
Can we change the default in the pop-up to No dasboard, but add to library?
Related to the last question, should we minimally change the default radio button selected depending on whether a user has any existing dashboards?

These are all great questions. Maybe @poffdeluxe can answer them. I think these need to be addressed from a Kibana standpoint and not uniquely in Maps. I am trying to make Maps as consistent with Lens as possible with regards to global chrome and dashboard works flows. The reason is that any differences between UIs take users out of their muscle memory flow and causes them to stutter/pause/think. A great tool is one that blurs into the background. UI differences shatter this blurring.

@nreese nreese requested a review from kindsun January 21, 2021 19:11
@kindsun
Copy link
Contributor

kindsun commented Jan 21, 2021

These are all great questions. Maybe @poffdeluxe can answer them. I think these need to be addressed from a Kibana standpoint and not uniquely in Maps. I am trying to make Maps as consistent with Lens as possible with regards to global chrome and dashboard works flows. The reason is that any differences between UIs take users out of their muscle memory flow and causes them to stutter/pause/think. A great tool is one that blurs into the background. UI differences shatter this blurring.

I'm all for consistency between apps, just so long as there's some clear plan to update the common component or allow customization of it so that it works as expected across apps with differing needs. No real objections to the updates on our end, just unsure if we should move forward with merging with the component as is. I guess I'll also be interested in what @poffdeluxe has to say.

One other odd feature is you can't save a new Map based on an old one in a different way. For example you can't:

  1. create a map
  2. save it to your library
  3. then either modify it or not and try to re-save it to a dashboard

The options stay greyed out and force you to save it the way the previous map was saved:

image

@nreese
Copy link
Contributor Author

nreese commented Jan 21, 2021

then either modify it or not and try to re-save it to a dashboard. The options stay greyed out and force you to save it the way the previous map was saved

Unchecking "Save as new map" will enable the "Add to dashboard" section.

@kindsun
Copy link
Contributor

kindsun commented Jan 21, 2021

Unchecking "Save as new map" will enable the "Add to dashboard" section.

Ah good catch, that's good then. Still a bummer we can't change how it's saved (why are library and dashboard mutually exclusive?) but I already mentioned that in the first bullet in #88759 (comment)

Copy link
Member

@nickpeihl nickpeihl left a comment

Choose a reason for hiding this comment

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

lgtm!

tested in firefox.

I think the Save to Dashboard workflow is fine, because it redirects to a dashboard in a dirty state (unsaved). The user is required to consciously save the changes to the dashboard.

Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

Agreed with @aaronjcaldwell comments, but in +1 because it's consistent with the rest of Kibana.

Having to check an additional radio-button is also a price payed only once, at the firsti save. If the map is saved-to-library, it will automatically continue to do that.

@nreese
Copy link
Contributor Author

nreese commented Jan 26, 2021

@elasticmachine merge upstream

@kmartastic
Copy link
Contributor

My 2 cents. I support the consistency in Kibana when creating new visualizations/maps.

Shouldn't a user be able to save it to a dashboard without redirecting? It's a pretty normal workflow to save your map periodically as you construct it without wanting to navigate away from the Maps app.

This is a good comment ^

The UX forces the user into one of two paths. If the user knows they want to build a dashboard, this is fine.
If a user wants to build a map... and the flexibility to add the map to a dashboard later, without the content management overhead, they cannot. That's not ideal. If this was a change in behavior from a previous version, it would be a problem.

We should not choose consistency without fully understanding the impact of that choice on real flows. This PR doesn't impede the customer, this PR improves and encourages the dashboard first flow, but it might annoy customers down the road when they want to clean up their library / manage their content.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
maps 2.6MB 2.6MB +1021.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
maps 138.3KB 138.5KB +243.0B

History

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

@nreese nreese merged commit 3938fa6 into elastic:master Jan 26, 2021
nreese added a commit to nreese/kibana that referenced this pull request Jan 26, 2021
* [Maps] allow saving maps to dashboards

* update saveMap functional test method

* update tags functional test

* review feedback

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
nreese added a commit that referenced this pull request Jan 26, 2021
* [Maps] allow saving maps to dashboards

* update saveMap functional test method

* update tags functional test

* review feedback

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants