Upgrade button-entity-row

ericmatte commented 5 years ago

Hi,

I really like the concept of your button-entity-row! However, there were some edge cases that I've encountered:

throwing "service required" even though I had explicitly set the service
not launching my custom script properly
icon/text not update when the state of the entity changes
etc.

So I've decided the rewrite the project to better suits my needs.

Here's what changed:

Migrate to lit-element
Simplify code
Fix edge cases
Update README.md
Add state icons (you can now select one icon for each entity state!)
Add support for more default services
Add Prettier to the project (VS-Code)
Remove depreciated icon_color
Cleanup & Refactor

I am making a PR here, in case you find my update (or parts of it) useful to your project :)

todo[bot] commented 5 years ago

|

https://github.com/custom-cards/button-entity-row/blob/0d895bda6b5faa81c7a3b2382888201cab963172/README.md#L28-L33

This comment was generated by todo based on a `TODO` comment in 0d895bda6b5faa81c7a3b2382888201cab963172 in #5. cc @ericmatte.

jcwillox commented 5 years ago

Wow, really great job, quite an overhaul you've done, I really like the ability to define state icons 👍 .

I have noticed a couple of issues:

The card now needs to be defined with a type of module instead of js when adding to Lovelace. This isn't documented anywhere and is a breaking change, also HACS still assumes it's type as js.
There is a breaking change with multiple rows of buttons.
The style attribute doesn't appear to work when trying to set css styling, it sets the buttons style attribute to "0: [object Object];" (shown below).
```
<paper-button style="0: [object Object];" class="button-inactive">
```
The following snippet no longer works with the error button config is not supported, this is because you are assuming the entity attribute as required, though a button should not require an entity.
```
  - icon: 'mdi:lightbulb-off-outline'
    service: homeassistant.turn_off
    service_data:
      entity_id: group.office_lights
```

ericmatte commented 5 years ago

Great, I'm glad to hear that you like it :D

The card now needs to be defined with a type of module instead of js when adding to Lovelace. This isn't documented anywhere and is a breaking change, also HACS still assumes it's type as js.

I was not aware of that, thanks. I will update the readme.

There is a breaking change with multiple rows of buttons.

I've removed it to simplify the code and because you can just call the button-entity-row multiple times (one for each row). But I can add it back without problem. That way, there will be no breaking change indeed.

The style attribute doesn't appear to work when trying to set css styling, it sets the buttons style attribute to "0: [object Object];" (shown below).

Will be fixed 👍

The following snippet no longer works with the error button config is not supported, this is because you are assuming the entity attribute as required, though a button should not require an entity.
      - icon: 'mdi:lightbulb-off-outline'
        service: homeassistant.turn_off
        service_data:
          entity_id: group.office_lights

I'll look into it!

ericmatte commented 5 years ago

I've fixed the issues that you have found.

I have also added that when you click on a button without any service (and no default service), it will open the hass-more-info modal for that entity (if there is an entityId on the button).

Complete changelog for https://github.com/custom-cards/button-entity-row/pull/5/commits/1b9b2d99458a12f0042ed51568aee849282c472f:

Change js to module in the README.md
Bring back the ability to have multiple rows within the same button-entity-row
entity field is back to optional
fix style returning [object Object]
Allow object and array of objects for service_data, style and state_icon
New feature: if no service given and their is no default service, show more info modal for that entity (if any)
Set style once at the config load
Prefix private methods with _

Let me know if there is anything else

jcwillox commented 5 years ago

Sweet! That was quick. Just gave it a test and everything works as expected.

I don't know if it's just me but the readme still says type: js? Also with regards to the readme, the 'Full Configuration Example' still has icon_color assuming we're removing it then this should be removed too.

The default colour for buttons has changed from primary-color to paper-item-icon-color this isn't necessarily an issue and does make sense as icons in home assistant use paper-item-icon-color though the mwc-button's (the inline buttons) use primary-color. So this is a bit of a toss-up? I feel a possible solution would to be able to specify the on/off colours?

We should also put a note somewhere in the readme for breaking changes after v0.2.0, e.g. removed icon_color, the card now needs to be defined with type: module. Assuming @paulbdavis is happy with this, ill also submit a PR to update the info.md with these changes.

ericmatte commented 5 years ago

We should also put a note somewhere in the readme for breaking changes after v0.2.0, e.g. removed icon_color, the card now needs to be defined with type: module.

@jcwillox, I think a CHANGELOG.md file would be more appropriate for this. Something like this. What do you think?

I don't know if it's just me but the readme still says type: js?

I was missing a commit. Now fixed 👍 https://github.com/custom-cards/button-entity-row/pull/5/commits/dc41ba920124da3b4c71869315ea39a42380d3b7

The default colour for buttons has changed from primary-color to paper-item-icon-color this isn't necessarily an issue and does make sense as icons in home assistant use paper-item-icon-color though the mwc-button's (the inline buttons) use primary-color. So this is a bit of a toss-up?

Imho, I think it is better to take advantage of the var(--paper-item-icon-active-color) and var(--paper-item-icon-color) colors, but I could also default it to var(--primary-color) in .button-default (when there is no on/off state). What do you think?

I feel a possible solution would to be able to specify the on/off colours?

For that, I have a couple of ideas, but I am not sure which one is the best:

Using hardcoded on_color and off_color attributes (less options):

- type: "custom:button-entity-row"
  buttons:
    - entity: switch.light_1
      on_color: yellow
      off_color: blue

Defining a state attribute in style (maybe a little confusing?):

- type: "custom:button-entity-row"
  buttons:
    - entity: switch.light_1
      style:
        - state: "on"
          color: yellow
          background: orange
        - state: "off"
          color: blue

Like state_icons, add a state_styles attributes (will take the styling from style and override props defined in the current state_styles)

- type: "custom:button-entity-row"
  buttons:
    - entity: switch.light_1
      style:
        background: lightgray
      state_styles:
        "on":
          color: yellow
          background: orange
        "on":
          color: blue

Once a solution is decided, I will update icon_color accordingly.

jcwillox commented 5 years ago

@jcwillox, I think a CHANGELOG.md file would be more appropriate for this. Something like this. What do you think?

Yeah, that's a good option, or I just realised we could probably put it in the release notes, looks like that's what others have been doing?

Like state_icons, add a state_styles attributes (will take the styling from style and override props defined in the current state_styles)

Nice 👍 ! I think on_color is probably less portable, and I agree to put the state attribute in style would be confusing. I was also thinking of something like state_styles, that seems like the best option to me. Plus it can also be used for entities that don't use on/off for their states.

Imho, I think it is better to take advantage of the var(--paper-item-icon-active-color) and var(--paper-item-icon-color) colors, but I could also default it to var(--primary-color) in .button-default (when there is no on/off state). What do you think?

That makes sense and the addition of state_styles would solve this, also leaving them the same is likely better for consistency.

paulbdavis commented 5 years ago

Looks good, glad for the update to LitElement, I have not gotten around to it.

If you can give me a summary of breaking changes I can merge this and tag a release

paulbdavis commented 5 years ago

Regarding the multi-row support, Doing multiple rows like in your example probably makes more sense, not sure why I implemented it that way in the beginning (maybe something with early lovelace that has since changed?)

I can't test it out at the moment, but is the newer method visually the same?

jcwillox commented 5 years ago

I can't test it out at the moment, but is the newer method visually the same?

The new method (having two button-entity-row's) leaves a slightly larger gap (8px margin) between the rows of buttons. But both methods still work 👍 .

ericmatte commented 5 years ago

Yes, so for the breaking changes:

Removed the depreciated icon_color attribute
Changed the type from js to module in the ReadMe.

CC @paulbdavis

ericmatte commented 5 years ago

The new method (having two button-entity-row's) leaves a slightly larger gap (8px margin)

Actually, the older method leaves you with the same gap, since the 8px padding si on each paper-button (which makes sense), as you can see in my update, but also in the current implementation: https://github.com/custom-cards/button-entity-row/blob/27b0f9c078155f5e136eaa1b0118c5537d8a2ac6/button-entity-row.js#L20

jcwillox commented 5 years ago

Actually, the older method leaves you with the same gap, since the 8px padding si on each paper-button (which makes sense), as you can see in my update, but also in the current implementation:

The 8px margin I meant was the one on each entity row applied by home assistant, you can see it on the main divs inside an entities card.

#states > * {
    margin: 8px 0px;
}

ericmatte commented 5 years ago

Oh right! That is true

ericmatte commented 5 years ago

@jcwillox I really like that new state_styles options 😍 (also added to the readme)

example-5

ericmatte commented 5 years ago

So unless, their is anything else you want me to add to this PR, my branch is now good to go 👍

jcwillox commented 5 years ago

@ericmatte Oh damn that looks sweet 😲 ! I just played around with state_styles and it's really good 👍

custom-cards / button-entity-row