-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Time to Visualize] Add visualizations to dashboard from save modal #83140
Conversation
src/plugins/visualize/public/application/components/visualize_listing.tsx
Outdated
Show resolved
Hide resolved
src/plugins/saved_objects/public/save_modal/saved_object_save_modal_dashboard.tsx
Outdated
Show resolved
Hide resolved
|
||
const { documentInfo, savedObjectsClient } = props; | ||
|
||
const fetchDashboards = useCallback( |
There was a problem hiding this comment.
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
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. |
@gchaps and I reviewed the copy and came up with the following: Tooltip3rd radio button & button label |
dfcbeb6
to
933a784
Compare
@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.
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 |
Can confirm that the unlink issue is present in Master. I have filed an issue for it #83864 and will fix it asap. |
9d24e74
to
3fa09f4
Compare
8c25ee5
to
89d934d
Compare
+1 to both of these suggestions. |
There was a problem hiding this 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> = ({ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
…thout specifying dashboard
Thanks for the feedback y'all! @ThomThomson @flash1293 @ryankeairns I made some changes to the behavior based on feedback:
Also, fixed a bug where you could click save when saving to an existing dashboard without specifying a dashboard to save to |
@elasticmachine merge upstream |
There was a problem hiding this 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 👏
src/plugins/saved_objects/public/save_modal/saved_object_save_modal.scss
Outdated
Show resolved
Hide resolved
src/plugins/visualize/public/application/components/visualize_listing.scss
Outdated
Show resolved
Hide resolved
…fdeluxe/kibana into feature/visualize-users-to-dashboard
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✔️ limits change.
There was a problem hiding this 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.
src/plugins/visualize/public/application/components/visualize_listing.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: gchaps <33642766+gchaps@users.noreply.github.com>
@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: |
…fdeluxe/kibana into feature/visualize-users-to-dashboard
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Distributable file count
Page load bundle
History
To update your PR or re-run it, just comment with: |
Summary
Implements #79925
This PR allows for visualizations to be saved directly to dashboards as by-value embeddables.
Checklist
Delete any items that are not applicable to this PR.
For maintainers