Close notification drawer after dismissing last notification#7229
Conversation
This change adds a listener to any changes of the notifications property once the drawer opens. After the last notification is dismissed, the notification drawer closes automatically, and the listener is removed.
| this.addEventListener( | ||
| "notifications-changed", | ||
| this._onNotificationsChanged | ||
| ); |
There was a problem hiding this comment.
We can call this in _computeNotifications instead of with an event? And otherwise we should add an observer on notifications instead of the notify
There was a problem hiding this comment.
The issue with all approaches that don't include an event observer is that the notifications are loaded async. That means when the user tries to open the drawer, for a split second notifications.length === 0 is true, and the drawer immediately closes again (actually stays closed).
This is an issue both if we use an observer pattern (like _openChanged does), and if we move this into _computeNotifications.
While encapsulating everything in _computeNotifications is a lot more compact, it runs into the issue mentioned above:
_computeNotifications(open, hass, notificationsBackend) {
if (!open) {
return [];
}
const configuratorEntities = Object.keys(hass.states)
.filter((entityId) => computeDomain(entityId) === "configurator")
.map((entityId) => hass.states[entityId]);
const notifications = notificationsBackend.concat(configuratorEntities);
// this evaluates to true when the user tries to open the drawer before we have
// received any notifications from the backend-subscription, thus preventing the drawer from opening
if (notifications.length === 0) {
this.open = false;
}
return notifications;
}The only way I have found to avoid this is by attaching the observer once we have loaded all notifications.
Another option would be to rewrite the whole component to set up the server subscription in PolymerElement.ready, but that would be a larger refactor that I did not feel comfortable proposing for this small feature.
There was a problem hiding this comment.
We should rewrite it to a LitElement, that will solve your problem and should be done anyway
There was a problem hiding this comment.
If you want that challenge that is :-)
There was a problem hiding this comment.
I have not worked with Polymer or LitElement before, so I'll have to see how deep I want to go into that rabbit hole for adding this mini-feature. 😄
Until then, is this PR not acceptable?
There was a problem hiding this comment.
I would not like to merge it as is, if you want to keep the Polymer you can use:
static get properties() {
return {
...,
notifications: {
type: Array,
computed: "_computeNotifications(open, hass, _notificationsBackend)",
observer: "_notificationsChanged",
},
...,
};
}
_notificationsChanged(newNotifications, oldNotifications) {
if (this.open && oldNotifications.length && !newNotifications.length) {
this.open = false;
}
}There was a problem hiding this comment.
I wasn't aware of the newNotifications, oldNotifications arguments to the observer function. This is a much easier approach. Thanks for the suggestion!
I adjusted the PR and propose merging like this. If I feel up for it, I'll create a separate PR for a LitElement rewrite, but don't want to commit to it at this time.
Using the observer pattern instead subscribing to change events for notification changes simplifies the implementation noticeably.
3 participants
Proposed change
This change adds a listener to any changes of the notifications property once the notification drawer opens. After the last notification is dismissed, the notification drawer closes automatically, and the listener is removed.
Type of change
Example configuration
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed: