-
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
Picture Glance Conversion to TS #2029
Conversation
Please do the conversion in a separate PR. |
I would also expect the title ("Living Room") to be vertically centered in the overlay, not baseline aligned with the icons. |
fb0cd8e
to
404f4b7
Compare
class="${ | ||
classMap({ | ||
"state-on": !STATES_OFF.has( | ||
this.hass!.states[entityConf.entity].state |
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 can be undefined.
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 might be easier to extract the render logic for each entity into a function so you can do checks if state obj exists etc.
const stateObj = this.hass.states[entityConf.entity]; | ||
|
||
if (!stateObj) { | ||
return html``; |
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 think we should render an error if this occurs as this means we have an invalid entity defined, correct? We should highlight that this entity is invalid in that case
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 did in the previous version, doesn't mean we shouldn't. I look into a yellow box and see what it looks like
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.
The problem is the pace is so tiny... Hard to give a good error
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.
what about a yellow box wrapped in a <paper-tooltip>
to show the warning when hovered?
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.
<div>
<ha-icon icon="hass:alert"></ha-icon></paper-tooltip>
<paper-tooltip>Unable to find ${entityConf.entity}</paper-tooltip>
</div>
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.
However, I am fine with sticking to the same logic as the element we're converting, and adding new logic in a future PR.
return; | ||
} | ||
|
||
if (this._config!.camera_image) { |
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 this be else if
? Otherwise we'll navigate AND open more info ?
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.
Well there is a return statement in the above but I converted to else if
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.
ooh my bad, didn't see.
Convert to TS