Trap sidebar links when page has a dirty editor #20161
Conversation
|
Very conceptual idea: I would implement this as an abstract class, let's say |
|
Sounds like it might be a neat idea but it's a little technically confusing to me, I'm not so familiar with routers and what they do. I do notice that if I return early from the update function of the hass-page-router it does appear to stay on the current page as desired, but the browser still acts as if it has moved on to the new page (the URL bar shows the new page we prevented loading, and it populates a new history item). Maybe that can be mitigated, but I'm not sure of the details. |
|
I'll see if I can put that idea into proof of concept code. |
|
I can't seem to let this work in the lit framework. It's quite hard to catch it whenever the page is being destructed since it destructs top down starting with the general config page layout, then as a child the automation picker and editor, but then it's too late to trap it and show a dialog. |
| } | ||
| const url = (ev.currentTarget as any).href; | ||
| ev.preventDefault(); | ||
| const leave = await this._confirmDirty(); |
There was a problem hiding this comment.
If we don't leave, we have to reset the selected panel in the listbox.
| @@ -224,6 +226,13 @@ class HaSidebar extends SubscribeMixin(LitElement) { | |||
| }) | |||
| private _hiddenPanels: string[] = []; | |||
|
|
|||
| @storage({ | |||
There was a problem hiding this comment.
Instead of using storage, cant we just send an event up from the automation page, this feature doesnt have to survive page reload etc?
There was a problem hiding this comment.
I think a context may be more suitable given the ha-sidebar is 3 levels higher than the automation page where we know if it's dirty or not?
There was a problem hiding this comment.
I think a context may be more suitable
Do you mind to elaborate a bit? I'm not very familiar with this concept.
There was a problem hiding this comment.
You can find more about it here: https://lit.dev/docs/data/context/
We already use it to prevent us bringing registries (like the device and entity registry) to be passed through each component. It's a bit the same like storages except it's kept in-memory thus not persistent, allowing to inject it into a components makes the context available.
The consumer will be the ha-sidebar, the provider should be the automation-editor component where _dirty is available to know if there are unsaved changes, and then lastly you have the context object which should keep the boolean value.
It's not the easiest thing from lit to get working unfortunately.
There was a problem hiding this comment.
The consumer should be higher in the tree than the provider I think, as it works with events.
Another thing that will work well for this probably is Signals. Currently a proposal with ponyfill: https://github.com/tc39/proposal-signals
There was a problem hiding this comment.
The consumer should be higher in the tree than the provider I think, as it works with events.
Are you saying sidebar cannot be the context consumer, as it is not strictly a parent element of the thing providing the context?
Tried adding @consume in ha-sidebar but never got it to pick up anything.
There was a problem hiding this comment.
Yeah, thats what I'm saying
There was a problem hiding this comment.
I did try doing this with events, and got it partially working, but I seem to have got stuck on one aspect.
I can send an event from automation page whether or not the page is dirty or clean, and that is working, but if I'm on the automation page and it is dirty, and I press the browser back button, then what seems to happen is the browser leaves the automation page, but the sidebar still thinks something is dirty so it is still showing the confirm dialog even though we're not on a dirty page anymore.
In the current proposal with @storage I can clear the dirty stored flag on disconnectedCallback when we're leaving the automation page for any reason. If I try firing an event to clear dirty from disconnectedCallback, it doesn't seem to reach anywhere, presumably because it's no longer connected anymore. So I don't know how to reset the dirty state when we're leaving a dirty page via browser back. (I can't show a confirm dialog on back button either, but I'm ignoring that for now)
… trap-dirty-sidebar
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Can we handle this on a more global level, so not just in the sidebar but by listening to the history API events? That way we can do something similar to the built in Have the element fire an event up, that it is dirty (or clean again) somewhere in the top level this is catched, when there is a |
|
Replaced by #23170 |
Proposed change
If automation editor is dirty, ask for a confirmation before following through any of the sidebar links.
I'll frequently edit an automation and want to hop over into devtools to test some template or check some state, only to realize a moment later that I've lost all my unsaved changes without warning.
I used a sessionStorage for this, though I'm not sure if that's the correct way to implement this kind of global lock or not.
Only added it to automation editor for the moment, but if we like it as a proof of concept should probably also be added to scripts, scenes, card editors, view editors, etc.
Type of change
Example configuration
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed: