-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Add overview summary for backups #23343
Conversation
}); | ||
fireEvent(this, "ha-refresh-backup-config"); |
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.
Maybe we should only do this on disconnected? As it will cause a lot of fetches?
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.
Yeah agree 🙂
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.
It can not be done in disconnected because it's already disconnected so the element will be fired.
Also, I don't think people will change these settings too often.
Should I try to improve it in another way or is it ok like that ?
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.
It is ok for now 👍🏻 would be nice if the update call would return the new config like other APIs do
@@ -196,10 +159,10 @@ class HaConfigBackupSettings extends LitElement { | |||
|
|||
private _encryptionKeyChanged(ev) { | |||
const password = ev.detail.value as string; | |||
this._backupConfig = { | |||
...this._backupConfig, | |||
this.config = { |
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.
I don't think we should change a public property, it could be overwritten by the parent before it is saved.
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.
It was a state before, I forgot to change this logic
this._backupConfig = config; | ||
protected firstUpdated(_changedProperties: PropertyValues): void { | ||
super.firstUpdated(_changedProperties); | ||
fireEvent(this, "ha-refresh-backup-config"); |
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.
Why is this needed?
protected willUpdate(changedProperties: PropertyValues): void { | ||
super.willUpdate(changedProperties); | ||
if (changedProperties.has("config")) { | ||
this._config = this.config; |
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.
Shouldn't it only do this if _config
is undefined?
Proposed change
Add overview summary card at the top
Move fetching and subscribe logic to backup root component.
Update icons
Type of change
Example configuration
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed: