Return to dashboard after editing embeddable #62865
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ThomThomson
added
release_note:enhancement
Feature:Dashboard
ThomThomson
force-pushed
the
dashboard/visEditRedirect
branch
from
62ba757 to
4060495
Compare
…of specific dashboard functionality. Changed save modal copy on save behavior
' argument. Changed the currentAppId observable into a behaviorSubject to make it easier to grab the current app Id from anywhere
…rom lens when redirect is not selected
ThomThomson
added
the
Team:Visualizations
Contributor
|
Pinging @elastic/kibana-app (Team:KibanaApp) |
ryankeairns
approved these changes
May 4, 2020
Contributor
There was a problem hiding this comment.
LGTM.
This is a really nice improvement and well executed. The 'Save as' dialog is a little challenging to sort through (mentally), but I was able to quickly figure out what each switch allowed me to do. In the end, it provides a lot of flexibility and does so within the confines of what had already existed.
Only one small thought that may/may not be worth changing. When in the 'Save as' dialog, should the button label be 'Save and return' when you have enabled the 'Add panel to Dashboard' switch? That would make it clearer that you about to navigate away from Visualize and would align with the other 'Save and return' button.
@elastic/kibana-canvas Check this out! If you create a new Visualization from Lens, then add it to your workpad (using the flyout), you can now use the 'Edit Lens' link from that new embedded element and (after editing) return to Canvas. Once Canvas adds the ability to 'Create new' visualization embeddable from Canvas, the circle will be complete. 😎
wylieconlon
approved these changes
May 4, 2020
Contributor
wylieconlon
left a comment
wylieconlon
left a comment
There was a problem hiding this comment.
Overall LGTM. Tested in Chrome in the following scenarios in both the default space and a custom space:
- Editing and saving Lens visualizations normally
- Adding a Lens visualization to a dashboard.
- Editing the previously saved visualization, using both "save as new" and not
- Adding Lens to Canvas, and editing
src/plugins/visualizations/public/embeddable/visualize_embeddable_factory.tsx
Outdated
Show resolved
Hide resolved
| let originitingAppParam = this.currentAppId; | ||
| // TODO: Remove this after https://github.com/elastic/kibana/pull/63443 | ||
| if (originitingAppParam === 'kibana') { | ||
| originitingAppParam += `:${window.location.hash.split(/[\/\?]/)[1]}`; |
Contributor
There was a problem hiding this comment.
Not a big fan of needing to know the exact positional order of the parameter in the URL: can you use one of the URL parsing libs we have, like query-string to convert this into a named argument?
Contributor
Author
There was a problem hiding this comment.
This whole section will thankfully not be necessary after @flash1293 is finished #63443. in the meantime, some URL parsing is necessary to grab dashboard out of:
#/dashboard/722b74f0-b882-11e8-a6d9-e546fe2bba5f ....
as far as I can see , it's not exactly possible to get a named argument for it using query-string, because it isn't really a param. That said, something like this:
const urlSplit = window.location.href.split('/');
editUrl += urlSplit[urlSplit.indexOf('kibana#') + 1]
Can isolate 'dashboard' without the exact positional order, but I'm not sure if the change is worth it. Any thoughts?
Contributor
Author
Great idea, got it in there! |
Dosant
reviewed
May 4, 2020
| ) {} | ||
| ) { | ||
| if (this.application?.currentAppId$) { | ||
| this.application.currentAppId$.subscribe( |
Contributor
There was a problem hiding this comment.
I think we have to somehow unsubscribe here, otherwise will get a memory leak
maybe we could just .pipe(first()) to make sure it unsubscribed itself (if that is sufficient)
same here: https://github.com/elastic/kibana/pull/62865/files#diff-85e9be7a6b60df87e48500d65f596d1aR101
Contributor
Author
There was a problem hiding this comment.
Oh, good eye. The subscription in edit_panel_action I've cleaned up by using a pipe as suggested.
The subscription in visualize_embeddable_factory is a little different, as it needs to update correctly when the app id changes. To unsubscribe that, I have a method in the factory called unsubscribeSubscriptions. The plugin keeps a reference to the factory, then calls that method in its stop method. This was the best way I could think of to clean up that subscription.
Contributor
There was a problem hiding this comment.
The this.application.currentAppId$ is actually a BehaviorSubject it always has a value and it can be accessed synchronously.
My guess is that you should be able to do
let originatingAppParam = getObservableCurrentValue(this.params.start().core.application.currentAppId$);where
const getObservableCurrentValue = (obs$) => {
let value;
obs$.pipe(take(1)).subscribe(anotherValue => value = anotherValue);
return value;
};🤞
Dosant
reviewed
May 4, 2020
Contributor
|
I'm wondering if this new functionality needs to be expressed via query params or if the same result could be achieved via functions exposed through plugin contracts. I don't see a reason from a browser perspective why it must be this way but we might be limited based on our existing toolset for navigating between apps. Anyway, I know its a disruptive thought that probably shouldn't alter the progress of this PR but I think its a question worth asking. |
|
Hey I played with the pr. |
streamich
approved these changes
May 5, 2020
Contributor
streamich
left a comment
streamich
left a comment
There was a problem hiding this comment.
Added few suggestions, but other than that AppArch code changes LGTM.
Comment on lines
+46
to
+48
| this.application.currentAppId$ | ||
| .pipe(take(1)) | ||
| .subscribe((appId: string | undefined) => (this.currentAppId = appId)); |
Contributor
There was a problem hiding this comment.
I've created a discuss issue to improve this syntax: #65224
Contributor
Author
There was a problem hiding this comment.
This is a great issue to bring up. I did switch currentAppId$ to a behaviorSubject at some point in this PR, but most tests aren't set up to ignore the first value and were failing, so I figured it would be better to do it in a separate PR.
| public async create() { | ||
| // TODO: This is a bit of a hack to preserve the original functionality. Ideally we will clean this up | ||
| // to allow for in place creation of visualizations without having to navigate away to a new URL. | ||
| let originatingAppParam = this.currentAppId; |
Contributor
There was a problem hiding this comment.
Instead of subscribing in constructor and having .unsubscribeSubscriptions() method, could this line be something like
Suggested change
| let originatingAppParam = this.currentAppId; | |
| let originatingAppParam = await this.getCurrentAppId(); |
Maybe it could even be synchronous
Suggested change
| let originatingAppParam = this.currentAppId; | |
| let originatingAppParam = getObservableValue(this.params.start().core.application.currentAppId$); |
Contributor
There was a problem hiding this comment.
Or may be inlining the pipe trick
let originatingAppParam;
core.application.currentAppId$
.pipe(take(1))
.subscribe(appId => originatingAppParam = appId);
Contributor
There was a problem hiding this comment.
I like @streamich 's suggestion here for using await instead of a subscription ☝️
Contributor
Author
There was a problem hiding this comment.
Implemented this suggestion!
| embeddable.registerEmbeddableFactory(VISUALIZE_EMBEDDABLE_TYPE, embeddableFactory); | ||
| this.embeddableFactory = new VisualizeEmbeddableFactory( | ||
| { start }, | ||
| async () => (await core.getStartServices())[0].application |
Contributor
There was a problem hiding this comment.
You should already have Application service in that start variable above.
start().core.application
Suggested change
| async () => (await core.getStartServices())[0].application |
Contributor
Author
There was a problem hiding this comment.
Implemented this suggestion. It's a lot cleaner now, thanks @streamich !
majagrubic
approved these changes
May 5, 2020
Contributor
There was a problem hiding this comment.
I tested this in Chrome 80 on Mac OS for following scenarios:
- editing existing Lens visualization
- editing existing regular visualization
- editing maps
- adding a new Lens visualization
- adding a new TSVB visualization
- all of the above but with session storage turned on
All work as expected.
Code looks good. I'd just apply that one suggestion from @streamich as mentioned above.
Just one UX comment:
I find the "Add to Dashboard" switch in the "Save Visualization" modal confusing - if the user had come from a dashboard, why would the user opt NOT to save edited visualization to the dashboard? And assuming this flow will go away by 7.9, it's even more confusing we're introducing this for just one minor? cc @AlonaNadler @ryankeairns
…ing create instead of managing a subscription
|
Great work @ThomThomson! Really awesome to see this, great tests too 👍 . I think you are aware of this and mention it, but the "edit" flow for canvas redirects back to landing page, not the workpad. Still less clicks to get back to where you were, anyway. 🍾 |
Contributor
Author
|
@elasticmachine merge upstream |
Contributor
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / kibana-xpack-agent / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/canvas/custom_elements·ts.Canvas app custom elements deletes custom element when promptedStandard OutStack TraceHistory
To update your PR or re-run it, just comment with: |
ThomThomson
added a commit
to ThomThomson/kibana
that referenced
this pull request
May 5, 2020
Changed the process for saving visualizations and lens visualizations when the user has been redirected there from another application.
Contributor
While not a primary use case, I viewed this as some added flexibility. I thought of it like this: I'm editing my Dashboard and I'd like to play around with a particular panel but not alter its content. I edit/open it in Visualize, freely change it, then 'Save as' and choose to not add it to the Dashboard... with no worry of messing things up. |
ThomThomson
added a commit
that referenced
this pull request
May 6, 2020
Changed the process for saving visualizations and lens visualizations when the user has been redirected there from another application. Co-authored-by: Elastic Machine <[email protected]>
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
11 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR changes the process for saving visualizations and lens visualizations when the user has been redirected there from another application.
How it works
The edit panel action and the visualize embeddable factory subscribe to
currentAppIdfrom the application service. When activated, they add a new param,embeddableOriginatingAppto the url which contains the current app id before redirecting to the edit url or getting explicit input. This allows for the below flow for dashboard panel adding & editing.Changes
The visualize and lens plugins read the
embeddableOriginatingAppparam and use it to help make the following changes.Top Nav
If the originating application param exists, and the embeddable has been saved before:
A new, emphasized Save and return button is added, which saves the visualization / lens visualization without showing the save modal, and immediately redirects back to the originating app.
The save button is renamed Save as, and when clicked, it opens the new save modal with the options Save as new visualization, and Add to dashboard after saving checked by default.
If the originating application param exists, and the embeddable has not been saved before:
If the originating application param does not exist:
Save modal
A new option has been added to the save modal, which either reads Add to dashboard after saving or Return to dashboard after saving.
If the embeddable is newly created, or Save as new visualization is checked, the button will be in Add mode, and saving will add the new embeddable onto the end of the dashboard.
If the embeddable is not newly created, the button will be in Return mode, and saving will return to the originating app without adding anything
Bonus
Because of the structure of this change, it also works correctly with canvas!
Note: when redirected from canvas, the save modal will hide Add to canvas after saving option when the save as new option is checked, because Canvas does not yet support adding an embeddable on redirect (Please correct me if I'm wrong!)
Checklist
Delete any items that are not applicable to this PR.
For maintainers