Skip to content

LoveLace Light Card#1874

Merged
zsarnett merged 6 commits intohome-assistant:devfrom
zsarnett:light-card
Oct 30, 2018
Merged

LoveLace Light Card#1874
zsarnett merged 6 commits intohome-assistant:devfrom
zsarnett:light-card

Conversation

@zsarnett
Copy link
Contributor

I put this together while not knowing what to do with a websocket call

image

image

Configuration Variables

Name Type Default Description
type string Required light
entity string optional Required light.my_light
name string friendly_name Name of card

Example

- type: light
  entity: light.living_room
- type: entity-button
  entity: light.bedroom
  name: Living Room

@ghost ghost assigned zsarnett Oct 27, 2018
@ghost ghost added the in progress label Oct 27, 2018
@balloob balloob changed the base branch from master to dev October 27, 2018 08:54
@balloob
Copy link
Member

balloob commented Oct 27, 2018

What happens if light supports no brightness?

@zsarnett
Copy link
Contributor Author

What happens if light supports no brightness?

If it has no brightness its set to 0% or off. The only time it should have no brightness. If the entity they are using has no brightness use the button card :)

@zsarnett
Copy link
Contributor Author

Made the Brightness Percentage above Name so that a longer name can be added.

@balloob
Copy link
Member

balloob commented Oct 28, 2018

Can you add a demo

@balloob
Copy link
Member

balloob commented Oct 28, 2018

https://deploy-preview-1874--home-assistant-lovelace-gallery.netlify.com/#demo-hui-light-card

When I click on the slider rail (not the slider knob), it toggles the light. That's not what I would expect. The toggle area of the icon should be limited to the icon only.

The slider knob is not centered on the rail:
image

I don't think that we should always show percentage. Maybe we can only show it when dragging as an overlay on the icon?

@zsarnett
Copy link
Contributor Author

zsarnett commented Oct 28, 2018

I can not replicate the clicking on the rail toggling the light effect

I see the effect in the gallery

@balloob
Copy link
Member

balloob commented Oct 29, 2018

The middle of the icon is not clickable because the brightness label still exists when not in use:

image

@balloob
Copy link
Member

balloob commented Oct 29, 2018

In demos, clicking on the rail works: http://roundsliderui.com/demos.html

In this implementation, it doesn't.

}

private _hideBrightness() {
brightnessTimout = setTimeout(() => {
Copy link
Member

Choose a reason for hiding this comment

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

You think that it's a good idea to share a timeout between all instances of this element?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. Meant to make a comment on the best way to do this. This is how it's done for the notification drawer but there is one of those.

Copy link
Member

Choose a reason for hiding this comment

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

what about an instance variable? this._brightnessTimeout

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm always so conflicted when creating properties in LitElement. I would need to set that as private _brightneesTimeout?: Any;

Copy link
Member

Choose a reason for hiding this comment

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

Never use any. Hover with your mouse over setTimeout, what is the defined returned value?

With TS you have to define the type like you said, then just assign it wherever you want yes.

return `brightness(${(brightness + 245) / 5}%)`;
}

private _computeColor(stateObj) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a type for light state objects like we have for climate?

Copy link
Member

Choose a reason for hiding this comment

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

marked as resolved but this state object does not have a type yet. Add types to all functions you define in your class.

padding-top: 40px;
font-size: 1.2rem;
}
.brightness {
Copy link
Member

Choose a reason for hiding this comment

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

The text is not always very visible as the icon color, which is the background of brightness, changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a stroke to it but not sure how material design esque this is

Copy link
Member

Choose a reason for hiding this comment

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

it's a good start, we can iterate on it.

@zsarnett
Copy link
Contributor Author

@balloob clicking the slider rail works in the demo of round slider because they aren't using a custom overlay for the tool tip. Because we are the overlay is above the rail and can't make it a higher z index with out overlaying a white background on our tool tip. At least when I try to change the zindex of the rail that's what I get

@frenck
Copy link
Member

frenck commented Oct 29, 2018

Just a small note: Could not find a related PR on the documentation repository.

@@ -0,0 +1,52 @@
#!/usr/bin/env node
Copy link
Member

Choose a reason for hiding this comment

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

Nice script.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ugh

implements LovelaceCard {
public hass?: HomeAssistant;
private _config?: Config;
private brightnessTimout?: any;
Copy link
Member

Choose a reason for hiding this comment

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

let's prefix names of private vars with _. I know it's "double", but we're also interacting with JS and they don't know the concept of private.

Copy link
Member

Choose a reason for hiding this comment

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

If you don't know the type, you can hover with your mouse on the function (if it's a built-in), otherwise use unknown. With unknown, you are not allowed to do any operation besides passing it on or comparing it, until you know the type. With any, anything goes and we're back to caveman untyped world.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha. I started to do that that but it was a NodeJS Type so I left it off. I added it in because it was accepted with no errors

private _setBrightness(e) {
this.hass!.callService("light", "turn_on", {
entity_id: this._config!.entity,
brightness_pct: e.value,
Copy link
Member

Choose a reason for hiding this comment

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

Now I know why the gallery is flaky. We don't have support for brightness_pct in our demo hass for the gallery.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How can I add that?

Copy link
Member

Choose a reason for hiding this comment

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

Where did you look?

Copy link
Member

Choose a reason for hiding this comment

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

implements LovelaceCard {
public hass?: HomeAssistant;
private _config?: Config;
private _brightnessTimout?: NodeJS.Timer;
Copy link
Member

Choose a reason for hiding this comment

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

We're running in a browser, not under NodeJS. Also:

image

number

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When changing to number I am getting Type Time is not assignable to Type Number here: this._brightnessTimout = setTimeout(() => {

I think Node is messing with things or something.. Because researching setTimeout shows it returns a "unique" number that is used when clearing the timeout

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix this by using window.setTimeout so node doesnt overload

Copy link
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

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

ok to merge when fixing type of setTimeout.

@zsarnett zsarnett merged commit 2262031 into home-assistant:dev Oct 30, 2018
@ghost ghost removed the in progress label Oct 30, 2018
@zsarnett zsarnett deleted the light-card branch October 30, 2018 03:39
@github-actions github-actions bot locked and limited conversation to collaborators Jul 7, 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.

5 participants