-
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] Transition Embeddable State Transfer to Session Storage #85688
[Time to Visualize] Transition Embeddable State Transfer to Session Storage #85688
Conversation
Pinging @elastic/kibana-presentation (Team:Presentation) |
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.
Tested this with creating a view visualizations and seems to work as advertised. The amount of tests seem like they'll catch a bunch of issues
@@ -20,6 +20,8 @@ | |||
import { Optional } from '@kbn/utility-types'; | |||
import { EmbeddableInput, SavedObjectEmbeddableInput } from '..'; | |||
|
|||
export const EMBEDDABLE_EDITOR_STATE_KEY = 'embeddable_editor_state'; |
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.
bit of a nit but this is more of a constant rather than a type, right? maybe could go in a constants files if applicable
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.
A constants file would be a good home for this, but the embeddable plugin doesn't have one yet.
💚 Build SucceededMetrics [docs]Async chunks
Distributable file count
Page load bundle
History
To update your PR or re-run it, just comment with: |
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.
Tested in Chrome and Firefox, transfer state seems to be reset in the relevant places (but take it with a grain of salt with regard to my track record in testing these changes :) )
However once the user starts to use the global search to navigate or to mess with the URL themselves, they can still end up in weird situations, e.g.:
- User goes to dashboard, creates new embedded Lens panel
- User uses global search to switch to another Library lens vis (or manually changes the URL)
- New vis loads, but connection to dashboard is still in place
I'm not 100% sure whether that's even a problem or just expected behavior - leaving that up to you :)
Approving if this was the intended flow
I noticed that these sorts of in-between cases were possible during implementation. They are not strictly intended, but they also don't particularly break anything - which is why I would be okay with merging this, and having a follow up to remove them. The problem stems from the state transfer state not being linked to the browser history in any way. If we clear the state upon returning to dashboard then it's cleared in all cases and the back / forward buttons won't restore it anymore. The only way I can think of to totally fix this is to introduce a short url param that determines whether the editor should check for incoming state or not. |
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.
AppServices code changes LGTM.
…torage (elastic#85688) * Transitioned embeddable state transfer service to use sessionStorage
Summary
closes #85493
Note - please ignore the pink header, It's there to easily distinguish between Kibana instances
This PR switches the Embeddable State Transfer service to use Session Storage. This means that the implicit behaviour of the state being reset to undefined is gone. Now the editor state is:
capable of persisting over reloads
Working correctly with the browser back and forward buttons
Additionally, this PR fixes the incompatibility between #82909 and #83140
Things to Remember When Testing:
dashboard.allowByValueEmbeddables: true
to yourkibana.dev.yml
Checklist
Delete any items that are not applicable to this PR.
For maintainers