Skip to content

Light hui & more-info card fixes#7397

Merged
bramkragten merged 5 commits intohome-assistant:devfrom
spacegaier:light-card-adjustments
Oct 20, 2020
Merged

Light hui & more-info card fixes#7397
bramkragten merged 5 commits intohome-assistant:devfrom
spacegaier:light-card-adjustments

Conversation

@spacegaier
Copy link
Member

@spacegaier spacegaier commented Oct 19, 2020

Breaking change

Proposed change

Fix various issues with the light more-info card:

  1. Previously the card size changed when the light was switched on/off
  2. Slider did not go down to 0, so you could not turn off the light with it
  3. Slider did not show a pin, since not the brightness percentage was used (instead the absolute value), but instead the default 0..255 values which is adjusted now as well
  4. Fixed that the brightness slider was not updated when the light was switched off

Global changes to <ha-labeled-slider>:

  1. Adjusted to use less space, by bringing label and slider nearer together
  2. Use the primary color for the slider label for better readability
  3. Show a pointer cursor if the pin is hovered to indicate "input possible"
  4. Ensured that a pin at position "min" also has a color

Before:

3E002F57-A6E0-442E-9E8D-C7A82AF6956B

After:

980BB235-02E8-4658-89E3-C60F339AEB47

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Comment on lines -69 to +70
min="1"
max="255"
min="0"
max="100"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By changing this the precision of the slider is now 2.55 times less

Copy link
Member

@bramkragten bramkragten Oct 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and we now have rounding differences, not sure those are issues but something to consider

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but is that something a user needs? Having the 0..100 percent is much more user friendly, plus it mirrors what we already use for the rounded slider in the hui-light-card.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hui-light-card however for contrast is only dividing by 254 before Math.round() compared that the 255 what I used now. Change made by you some time ago (#4257).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't change the logic there, just moved the line ;-)

Copy link
Member

@bramkragten bramkragten Oct 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 254 is from the original light card #1874 think we should change that to 255.

@iantrich
Copy link
Member

I'm not in favor of the slider going to 0. More often than not, when using the slider one wants the min or max for the light. Now, selecting the min will take some precision to avoid turning the light off when there is a switch right there to turn it off.

@spacegaier
Copy link
Member Author

I'm not in favor of the slider going to 0. More often than not, when using the slider one wants the min or max for the light. Now, selecting the min will take some precision to avoid turning the light off when there is a switch right there to turn it off.

Yeah, I saw your comment in the old linked PR 😄. Is there really a use case for manually dimming a light to 1% or 99% specifically?

Either way, my priority is to make it consistent. The light card with the round slider is currently going down to zero and switching off the light. The more-info card should follow that or we should change both to use 1..99 instead of 0..100.

=> How do we make a decision? So far the opinions seem to be rather split in half.

@thomasloven
Copy link
Contributor

I'm with Ian. In the middle of the night I often want to quickly turn on the light but as little as possible.

@Mariusthvdb
Copy link
Contributor

please allow me to add this:
with the now defunct custom-ui we could set this as an option:
Schermafbeelding 2020-10-20 om 10 30 39

which was rather cool. It offers both options on a per light demand case. Dont set this on the night light you want to minimize by sweeping it to the left (as Thomas needs, and dont we all .;-) )
for numerous other lights this can be of utmost convenience, even when having the toggle available.

Hope you will consider, it is a long standing wish, at least for the community that used the custom-ui before.

And, once you have this available, you will start liking it ;-)

@noxhirsch
Copy link
Contributor

noxhirsch commented Oct 20, 2020

I'm with Ian, too. Additionally to the already stated reasons I mostly open the more-info dialog only for brightness (1-255 / 1-100) or history. If I want to switch off a light very quick, I directly tap on the switch in the entity row.
EDIT: I know that this change primarily for the more-info card. My argument is only for the dialog, which is also affected. For the card everything is already said above ;)

@bramkragten
Copy link
Member

@Mariusthvdb please stop plugging custom ui features everywhere, we won't add that kind of option, especially not with the config bound to the entity.

@Mariusthvdb
Copy link
Contributor

Mariusthvdb commented Oct 20, 2020

Sorry to have believed this was still in discussion.... I was merely responding to the @spacegaier s post where he mentioned this option.
I now see you are not willing to even think about this common use case.

stop plugging custom ui features everywhere

really baffled by your response here. seems a bit harsh doesn't it?

@bramkragten
Copy link
Member

He never mentioned that it could be an option, just that it should be consistent?

It might sound harsh, but PR's are simply not the place to request features. Also, there is a reason that custom UI was custom and not in core.

@bramkragten
Copy link
Member

I agree the slider should stay at min 1, if you want to turn it off you can just click the switch, that is even faster and easier.

@Mariusthvdb
Copy link
Contributor

He never mentioned that it could be an option

I responded to:

Is there really a use case for manually dimming a light to 1% or 99% 

and with my suggestion, opted there are valid uses cases for both situations.

there is a reason that custom UI was custom and not in core.

Well, that's a bit of a bold view.
One could also state core HA doesn't (yet) offer the options and flexibility end-users need for the Home-automation system to be really life-like.. Hence the willingness of the many people in the HA community trying to spend time in the HA product, and create/find and offer solutions for that.
Merely disdaining Custom solutions is not very openminded. Nor is it encouraging to that community.

Enough said, let's make HA a(n even) better product.

@iantrich
Copy link
Member

The light card with the round slider is currently going down to zero and switching off the light

@spacegaier I don't personally use the light-card so did not notice that, but that should also have a min of 1 IMO

@zsarnett
Copy link
Contributor

I do think it should be a percentage and not 1-255. More user friendly. No one needs that type of detail and if they do run an automation

@spacegaier
Copy link
Member Author

spacegaier commented Oct 20, 2020

I adjusted the PR based on the majority vote: Percentage going from 1 to 100 for the sliders + fixed a general issue with the pin coloring.

@spacegaier spacegaier changed the title Light more-info card fixes Light hui & more-info card fixes Oct 20, 2020

const brightness =
Math.round((stateObj.attributes.brightness / 254) * 100) || 0;
Math.round((stateObj.attributes.brightness / 255) * 100) || 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be 1 now?

Suggested change
Math.round((stateObj.attributes.brightness / 255) * 100) || 0;
Math.round((stateObj.attributes.brightness / 255) * 100) || 1;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure, but I think no. If for some reason we get invalid brightness value so that the first part results in an NaN, then we should not switch on the light slightly dimmed at 1%, but it should stay at 0. Or am I missing something here?

@bramkragten bramkragten merged commit 2be08ce into home-assistant:dev Oct 20, 2020
@spacegaier spacegaier deleted the light-card-adjustments branch October 20, 2020 22:00
@bramkragten bramkragten mentioned this pull request Oct 21, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Jul 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants