Control modification for cover#298
Conversation
|
Can you attach some screenshots of different configurations? I.e. An entity that only supports open/close, an entity that only tilts, one that does both. Also be sure to check a small width card. (Mobile) |
|
I'll add this a bit later when I go back home.
…On Wed, May 31, 2017 at 8:56 AM, Adam Mills ***@***.***> wrote:
Can you attach some screenshots of different configurations? I.e. An
entity that only supports open/close, an entity that only tilts, one that
does both. Also be sure to check a small width card. (Mobile)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#298 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABjyfagdv2xwW6yZljn8aMYsB9Tx0MBlks5r_WOIgaJpZM4NrGu9>
.
|
|
@armills here are the screenshots
|
|
OK, thanks. So I think we should only show one set of controls on the state card. It's too crowded otherwise. I think what we should do is if there are tilt controls but no cover controls, we show the tilt controls. Otherwise we only show the cover controls. I believe this was the original use case, right? |
| .has-close_tilt .tilt, | ||
| .has-stop_tilt .tilt, | ||
| { | ||
| max-height: 40px; |
There was a problem hiding this comment.
The max-height classes here are redundant with the invisible attribute. We use max-height on the more info dialog so that hidden controls collapse vertically, but here the buttons should just occupy their space anyway, so visibility is enough.
|
No. Original case was that only cover controls were shown and configuration was required. Tilt controls were only shown on the more-info card. I'd definitely need some more guidance/help if the either/or situation was put in place. The controls are separate components now and I have no idea how I'd keep them logically separated if they had to "know" if the other were shown or not. The same goes for the more info card; there would have to be logic here to put the tilt controls into the body of the card if they weren't in the header. |
|
I think it shouldn't be too bad. What I'm thinking is in |
| var openClasses = [ | ||
| window.hassUtil.featureClassNames(stateObj, this.openFeatureClassNames), | ||
| ]; | ||
| var openClassString = openClasses.join(' ').trim(); |
There was a problem hiding this comment.
This is pretty clever, but I think we should only be doing integer or logical checking here rather than generating these strings, joining them, and looking for an empty string. I'm thinking something along these lines.
var supportsCover = this.supportsOpen || this.supportsClose || this.supportsStop;
var supportsTilt = this.supportsOpenTilt || this.supportsCloseTilt || this.supportsStopTilt;
return supportsTilt && !supportsCover;or
var supportsCover = this.stateObj.attributes.supported_features & 11; //where 11 is in a constant somewhere
emlove
left a comment
emlove
left a comment
There was a problem hiding this comment.
Code LGTM. Do you mind updating the screenshots to reflect the latest code?
|
Delete the old ones or just put new ones at the bottom?
…On Wed, May 31, 2017 at 3:51 PM, Adam Mills ***@***.***> wrote:
***@***.**** approved this pull request.
Code LGTM. Do you mind updating the screenshots to reflect the latest code?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#298 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABjyfVAhg1dNLoEXvVzG0QqnQ-sHtF3_ks5r_cS2gaJpZM4NrGu9>
.
|
|
Either way is fine. |
|
From top to bottom: More info for tilt only More info for open/close and tilt |
|
So just coming in here to say that 3 buttons is the max that fits in a state info but it seems like you already fixed that 👍 |
Grammatical changes for the English language. Co-authored-by: Daniel Shokouhi <dshokouhi@gmail.com>
4 participants
Modifying cover card to have up/down and/or tilt controls on the face of the card.
Relies on home-assistant/core#7841