-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Show the user that made the change in logbook #6173
Show the user that made the change in logbook #6173
Conversation
Co-authored-by: Bram Kragten <mail@bramkragten.nl>
Co-authored-by: Bram Kragten <mail@bramkragten.nl>
src/panels/logbook/ha-logbook.ts
Outdated
private async _fetchUsers() { | ||
const users = await fetchUsers(this.hass); | ||
users.forEach((user) => { | ||
this._userid_to_name[user.id] = user.name; |
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 doesn't update now after the users are loaded
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.
You could do this is a temporary object and after the forEach set it to this._userid_to_name
that will make sure we do a re-render.
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'm not following. Is the concern that we remove users and then the user ids are still in this._userid_to_name
?
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.
No, LitElement only re-render if a property is changed, it does this with a strict check, if we add things to this._userid_to_name
it will not change this._userid_to_name
.
So the LitElement will not re-render, thus not showing names until something else changed that will make it rerender.
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.
This btw would only be a problem if the logbook data is loaded before the user data, that is probably never going to happen but you are well on your way...
src/panels/logbook/ha-logbook.ts
Outdated
private async _fetchUsers() { | ||
const users = await fetchUsers(this.hass); | ||
users.forEach((user) => { | ||
this._userid_to_name[user.id] = user.name; |
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.
Other option would be to copy it: this._userid_to_name = { ...this._userid_to_name, user.id: user.name };
But that is making a lot of copies.
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.
WE should make it a @property, create a new object here and just assign it to
this._userid_to_name = userid_to_name`
Proposed change
Show the user that made the change in logbook
Type of change
Example configuration
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed: