Skip to content
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.

Sign up for GitHub

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

Trap sidebar links when page has a dirty editor #20161

Closed
wants to merge 2 commits into from
Closed

Trap sidebar links when page has a dirty editor #20161

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3ade1fb
Trap sidebar links when page has a dirty editor
karwosts Mar 22, 2024
a535893
Merge branch 'dev' of https://github.com/home-assistant/frontend into…
karwosts Apr 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 42 additions & 1 deletion src/components/ha-sidebar.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import { customElement, eventOptions, property, state } from "lit/decorators";
import { classMap } from "lit/directives/class-map";
import memoizeOne from "memoize-one";
import { storage } from "../common/decorators/storage";
import { navigate } from "../common/navigate";
import { fireEvent } from "../common/dom/fire_event";
import { toggleAttribute } from "../common/dom/toggle_attribute";
import { stringCompare } from "../common/string/compare";
Expand All @@ -49,6 +50,7 @@ import { UpdateEntity, updateCanInstall } from "../data/update";
import { SubscribeMixin } from "../mixins/subscribe-mixin";
import { actionHandler } from "../panels/lovelace/common/directives/action-handler-directive";
import { haStyleScrollbar } from "../resources/styles";
import { showConfirmationDialog } from "../dialogs/generic/show-dialog-box";
import type { HomeAssistant, PanelInfo, Route } from "../types";
import "./ha-icon";
import "./ha-icon-button";
Expand Down Expand Up @@ -224,6 +226,13 @@ class HaSidebar extends SubscribeMixin(LitElement) {
})
private _hiddenPanels: string[] = [];

@storage({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using storage, cant we just send an event up from the automation page, this feature doesnt have to survive page reload etc?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a context may be more suitable

Do you mind to elaborate a bit? I'm not very familiar with this concept.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@bramkragten bramkragten Apr 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, thats what I'm saying

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

key: "pageDirty",
state: false,
subscribe: true,
})
private _pageDirty = false;

public hassSubscribe(): UnsubscribeFunc[] {
return this.hass.user?.is_admin
? [
Expand Down Expand Up @@ -289,6 +298,7 @@ class HaSidebar extends SubscribeMixin(LitElement) {

protected firstUpdated(changedProps: PropertyValues) {
super.firstUpdated(changedProps);
this._pageDirty = false;
subscribeNotifications(this.hass.connection, (notifications) => {
this._notifications = notifications;
});
Expand Down Expand Up @@ -430,6 +440,7 @@ class HaSidebar extends SubscribeMixin(LitElement) {
href=${`/${urlPath}`}
data-panel=${urlPath}
tabindex="-1"
@click=${this._confirmNavigate}
@mouseenter=${this._itemMouseEnter}
@mouseleave=${this._itemMouseLeave}
>
Expand Down Expand Up @@ -545,6 +556,7 @@ class HaSidebar extends SubscribeMixin(LitElement) {
href="/config"
data-panel="config"
tabindex="-1"
@click=${this._confirmNavigate}
@mouseenter=${this._itemMouseEnter}
@mouseleave=${this._itemMouseLeave}
>
Expand Down Expand Up @@ -615,6 +627,7 @@ class HaSidebar extends SubscribeMixin(LitElement) {
tabindex="-1"
role="option"
aria-label=${this.hass.localize("panel.profile")}
@click=${this._confirmNavigate}
@mouseenter=${this._itemMouseEnter}
@mouseleave=${this._itemMouseLeave}
>
Expand Down Expand Up @@ -661,13 +674,41 @@ class HaSidebar extends SubscribeMixin(LitElement) {
: ""}`;
}

private _handleExternalAppConfiguration(ev: Event) {
private async _handleExternalAppConfiguration(ev: Event) {
ev.preventDefault();
if (this._pageDirty && !(await this._confirmDirty())) {
return;
}
this.hass.auth.external!.fireMessage({
type: "config_screen/show",
});
}

private async _confirmDirty(): Promise<boolean> {
const confirmed = await showConfirmationDialog(this, {
text: this.hass!.localize("ui.sidebar.confirm_unsaved"),
confirmText: this.hass!.localize("ui.common.leave"),
dismissText: this.hass!.localize("ui.common.stay"),
destructive: true,
});
if (confirmed) {
this._pageDirty = false;
}
return confirmed;
}

private async _confirmNavigate(ev: CustomEvent) {
if (!this._pageDirty) {
return;
}
const url = (ev.currentTarget as any).href;
ev.preventDefault();
const leave = await this._confirmDirty();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't leave, we have to reset the selected panel in the listbox.

if (leave) {
navigate(url);
}
}

private get _tooltip() {
return this.shadowRoot!.querySelector(".tooltip")! as HTMLDivElement;
}
Expand Down
13 changes: 12 additions & 1 deletion src/panels/config/automation/ha-automation-editor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import { property, state } from "lit/decorators";
import { classMap } from "lit/directives/class-map";
import { fireEvent } from "../../../common/dom/fire_event";
import { navigate } from "../../../common/navigate";
import { storage } from "../../../common/decorators/storage";
import { afterNextRender } from "../../../common/util/render-status";
import "../../../components/ha-button-menu";
import "../../../components/ha-fab";
Expand Down Expand Up @@ -98,7 +99,12 @@ export class HaAutomationEditor extends KeyboardShortcutMixin(LitElement) {

@state() private _config?: AutomationConfig;

@state() private _dirty = false;
@storage({
key: "pageDirty",
state: true,
subscribe: false,
})
private _dirty = false;

@state() private _errors?: string;

Expand All @@ -117,6 +123,11 @@ export class HaAutomationEditor extends KeyboardShortcutMixin(LitElement) {

private _configSubscriptionsId = 1;

disconnectedCallback() {
super.disconnectedCallback();
this._dirty = false;
}

protected render(): TemplateResult {
const stateObj = this._entityId
? this.hass.states[this._entityId]
Expand Down
3 changes: 2 additions & 1 deletion src/translations/en.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1798,7 +1798,8 @@
"sidebar_toggle": "Sidebar toggle",
"done": "Done",
"hide_panel": "Hide panel",
"show_panel": "Show panel"
"show_panel": "Show panel",
"confirm_unsaved": "[%key:ui::panel::config::common::editor::confirm_unsaved%]"
},
"panel": {
"my": {
Expand Down
Loading