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

[Time to Visualize] Add visualizations to dashboard from save modal #83140

Merged

Conversation

poffdeluxe
Copy link
Contributor

@poffdeluxe poffdeluxe commented Nov 11, 2020

Summary

Implements #79925

This PR allows for visualizations to be saved directly to dashboards as by-value embeddables.

  • Add "double-wide" / right options prop to the saved objects modal for extended options
  • New save flow is only available if by-value embeddables feature flag is enabled
  • New flow does not appear or get activated if user is editing the visualization within another app's editing flow (For example, if user is on a dashboard and edits a visualization in the panel)
  • New flow allows user to search existing dashboards to place visualization
    • The Dashboard Picker component is available as a standalone component for future use
  • New flow allows user to save new visualization on a new dashboard
  • New flow is deactivated if editing an existing visualization and "Save as new copy" is deactivated. The dashboard options are re-enabled if the visualization is being saved as copy

Lens_-_Elastic

Checklist

Delete any items that are not applicable to this PR.

For maintainers


const { documentInfo, savedObjectsClient } = props;

const fetchDashboards = useCallback(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

could probably do with some debouncing here if we're going to call this when a user is typing

@ryankeairns
Copy link
Contributor

Really great start here! 🥇

The overall experience is feeling good, and we can iterate on the flow for existing visualizations. It may be too presumptive defaulting to 'Save as new visualization', but I'm curious to hear how others feel.

Design-wise it's also in good shape, and I'm happy to help sand off edges once you're ready.

@ryankeairns
Copy link
Contributor

@poffdeluxe

@gchaps and I reviewed the copy and came up with the following:

Tooltip

Screen Shot 2020-11-12 at 11 31 20 AM

3rd radio button & button label

Screen Shot 2020-11-12 at 11 31 04 AM

@poffdeluxe poffdeluxe force-pushed the feature/visualize-users-to-dashboard branch from dfcbeb6 to 933a784 Compare November 17, 2020 21:31
@AlonaNadler
Copy link

👏 👏
The new visualize module works really really well, it’s fantastic!! The transition from visualize to dashboard new/existing is great and will help users avoid moving explicitly between apps

Few issues I saw (probably small bugs):

  • If I unlink from the library, edit in Lens, make changes and click save and return. Even though the panel is not tied to the library the changes are applied to the library copy

Nov-18-2020 15-03-47

  • When selecting a new dashboard it was unclear that it doesn’t sve the dashboard. I think since I enter the name of the object I thought the dashboard was also saved. Eventually, I lost work. Not sure if you have an idea on how to solve it or whether we should give it move time to learn if it is a real issue. In the past, we discussed a pop that suggests saving before moving from the dashboard
  • 2 cases where I lost something I just added to the dashboard:
    lost 1:
    lost1

lost2:
lost2

@poffdeluxe
Copy link
Contributor Author

@AlonaNadler Thanks for all this feedback! For the unlinking issue, that looks like it's an issue even in master. Looking into it. The two lost examples seem to both be cases where the embeddable gets added to the dashboard but the dashboard doesn't get saved. I just pushed a change so that when you go to the dashboard, the dashboard is put into its "edit state" to prompt the user to save the new embeddable to the dashboard.

When selecting a new dashboard it was unclear that it doesn’t sve the dashboard. I think since I enter the name of the object I thought the dashboard was also saved. Eventually, I lost work. Not sure if you have an idea on how to solve it or whether we should give it move time to learn if it is a real issue. In the past, we discussed a pop that suggests saving before moving from the dashboard

I spoke to @ThomThomson briefly about this today and I think there are some additional dashboard changes coming soon that will warn a dashboard user if they have those unsaved changes. I think just having this pop up will really alleviate those issues you're seeing

@ThomThomson
Copy link
Contributor

Can confirm that the unlink issue is present in Master. I have filed an issue for it #83864 and will fix it asap.

@poffdeluxe poffdeluxe force-pushed the feature/visualize-users-to-dashboard branch from 9d24e74 to 3fa09f4 Compare November 20, 2020 20:35
@flash1293
Copy link
Contributor

flash1293 commented Dec 4, 2020

On editing an existing visualization and unchecking "Save as new visualization", the button still says "Save and add to library"
Screenshot 2020-12-04 at 15 56 30

That's a little confusing IMHO because it's already added to the library. What about just using "Save" here.

This is just my personal opinion, I'm sure it has been discussed, but I still wanted to mention it: I think it's a little too aggressive in pushing for dashboard first to default to save an existing "Library" vis to a dashboard instead of updating the existing library vis. At this point it seems like the user is updating an existing vis on purpose, so IMHO it should default to just save it.

@ryankeairns
Copy link
Contributor

That's a little confusing IMHO because it's already added to the library. What about just using "Save" here.

This is just my personal opinion, I'm sure it has been discussed, but I still wanted to mention it: I think it's a little too aggressive in pushing for dashboard first to default to save an existing "Library" vis to a dashboard instead of updating the existing library vis. At this point it seems like the user is updating an existing vis on purpose, so IMHO it should default to just save it.

+1 to both of these suggestions.

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Lens code looks pretty good to me already, just left a small comment

onSave: (props: OnSaveProps & { dashboardId?: string | null; newTags?: string[] }) => void;
};

export const TagEnhancedSavedObjectSaveModalDashboard: FC<TagEnhancedSavedObjectSaveModalDashboardProps> = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

This file shares a lot of code with x-pack/plugins/lens/public/app_plugin/tags_saved_object_save_modal_origin_wrapper.tsx - can we somehow unify them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about that, for sure. My biggest concern is I kinda want to make it easy to back out of changes if we have to end up deciding to make big adjustments to the time-to-visualize flow after we release. I can def go either way here, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, then let’s move the “renderModal” logic into a separate component which calls the respective modal in the right scenarios. This keeps that stuff out of the app component (it already way too big anyway) and doesn’t over-abstract stuff which will likely change in the future.

@poffdeluxe
Copy link
Contributor Author

Thanks for the feedback y'all!

@ThomThomson @flash1293 @ryankeairns I made some changes to the behavior based on feedback:

  1. If you're editing an existing visualization, it defaults the "copy on save" to false and the button's text will just read "Save"
  2. If you're editing an existing visualization and you turn copy on save to true, then the adding to library text will then save "Save and Add to Library"

Also, fixed a bug where you could click save when saving to an existing dashboard without specifying a dashboard to save to

@poffdeluxe
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

This turned out really nice, works as expected, and it will certainly help people understand the library/Dashboard-first approach 👏

Copy link
Contributor

@tylersmalley tylersmalley left a comment

Choose a reason for hiding this comment

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

✔️ limits change.

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

The Lens part looks mostly good to me, I'm going to approve to unblock this PR.

However I noticed one weird case in the flow (not sure whether expected).

  • Edit an embedded Lens vis by clicking "Edit panel" from the dashboard
  • The breadcrumb says "Dashboard / Visualize / < name >"
  • Save the visualization to the library via "Save as"
  • Now the breadcrumb says "Visualize / < name >" - to me this looks like the connection to the dashboard has been severed and I'm in "Library mode" now, but that's not the case, because...
  • If i'm hitting save again it tries to save it to the dashboard again, probably because it still remembers the originating app

Not necessarily a bug, but it struck me as weird.

Co-authored-by: gchaps <33642766+gchaps@users.noreply.github.com>
@ThomThomson
Copy link
Contributor

@flash1293, looks like this edge case is also in master. Not sure when this started happening, but I remember it working properly back when Lens by Value was written. I've written a one line fix for it. Not the most elegant thing in the world, but it fixes the issue:

#85309

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lens 458 460 +2
presentationUtil - 18 +18
total +20

Async chunks

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

id before after diff
dashboard 196.9KB 197.0KB +52.0B
lens 1004.0KB 1012.0KB +8.0KB
visualize 127.6KB 130.8KB +3.1KB
total +11.2KB

Distributable file count

id before after diff
default 46923 47690 +767
oss 27565 27572 +7

Page load bundle

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

id before after diff
lens 52.9KB 53.1KB +243.0B
presentationUtil - 25.3KB +25.3KB
savedObjects 80.0KB 81.0KB +1022.0B
visualize 40.1KB 40.4KB +243.0B
total +26.8KB

History

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

@poffdeluxe poffdeluxe merged commit 8467b89 into elastic:master Dec 8, 2020
@poffdeluxe poffdeluxe deleted the feature/visualize-users-to-dashboard branch December 8, 2020 21:39
poffdeluxe added a commit that referenced this pull request Dec 9, 2020
…83140) (#85345)

Co-authored-by: Ryan Keairns <contactryank@gmail.com>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: gchaps <33642766+gchaps@users.noreply.github.com>
# Conflicts:
#	packages/kbn-optimizer/limits.yml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:medium Medium Level of Effort release_note:skip Skip the PR/issue when compiling release notes review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants