[Dashboard] Add visualization by value to Dashboard#68902
[Dashboard] Add visualization by value to Dashboard#68902majagrubic wants to merge 21 commits intoelastic:masterfrom
Conversation
|
@elasticmachine merge upstream |
bece04b to
cd6735d
Compare
|
Pinging @elastic/kibana-app (Team:KibanaApp) |
ppisljar
left a comment
There was a problem hiding this comment.
cool! i think we should throw some things around but its very close.
There was a problem hiding this comment.
should we navigate to empty dashboard if one was not loaded ? should we allow for passing dashboardId to navigateToDashboard function ?
There was a problem hiding this comment.
could we rather say that input can also be undefined, and if its undefined we show the modal else we create from the input
There was a problem hiding this comment.
input won't be undefined in case of a "regular save", it contains timeRange and query and a few other parameters:
src/plugins/visualizations/public/embeddable/visualize_embeddable_factory.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
i think we should not be loading the dashboard and manipulating it in visualize (this requires internal knowledge about dashboard structure)
rather we should navigate to dashboard and let dashboard take care of that:
navigateToApp('dashboard', { newVis: vis.serialize(); })
and then dashboard should check for newVis on its state, if its there it can append it to its panels
There was a problem hiding this comment.
I agree. The problem here is that we started working on a better inter-app communication logic and the PR hasn't been merged yet. As I mentioned in the description, this code is definitely going to change after #67064 is merged, and what you're describing is pretty similar to how it is going to work. We'll navigate to dashboard and pass the serialized vis as input, and the dashboard is going to render.
I did not want to base this PR off an unmerged PR, but I also didn't want to be blocked by that change.
I think this way of communicating between dashboard and visualization is good enough for now.
12addcd to
0bb50b9
Compare
ppisljar
left a comment
There was a problem hiding this comment.
code LGTM, looking forward to improved communication between visualize and dashboard as well.
stratoula
left a comment
There was a problem hiding this comment.
Code LGTM. I was more thorough on the vis editor code. Tested it on Chrome and it works as expected.
💔 Build Failed
Failed CI StepsBuild metrics@kbn/optimizer bundle module count
History
To update your PR or re-run it, just comment with: |
|
Thanks @ppisljar , @stratoula and @ThomThomson for your reviews. I'm closing this PR in favor of: #69898 and will ping you for another review there 😃 |
Summary
This is the first pass in adding Visualization to Dashboard by value. It should be fully functional. However, this solution is based on passing dashboard input to visualize editor, and then using this input to pass visualization data back to dashboard, which is a bit hacky (and won't work in case of a page reload). I will change this logic after #67064 is merged.
I'm hiding the new flow behind a "feature flag" by utilizing the server-side config. The current flow should remain unaffected. In order to test this locally, the value of the
showNewVisualizeFlowflag needs to be set to "true" insrc/plugins/visualize/config.tsandsrc/plugins/visualize/server/index.ts.Checklist
Delete any items that are not applicable to this PR.
- [] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support- [ ] Documentation was added for features that require explanation or tutorialsFor maintainers