Skip to content

Add image type for picture-elements card.#1428

Merged
balloob merged 3 commits into
home-assistant:masterfrom
jeradM:image-elements
Jul 12, 2018
Merged

Add image type for picture-elements card.#1428
balloob merged 3 commits into
home-assistant:masterfrom
jeradM:image-elements

Conversation

@jeradM
Copy link
Copy Markdown
Member

@jeradM jeradM commented Jul 9, 2018

Depends on #1423

Allows adding images to a picture-elements card
Closes: home-assistant/ui-schema#75

Example using state_image and state_filter:

- type: image
  entity: light.living_room
  tap_action: toggle
  image: /local/living_room.png
  state_image:
    'off': /local/living_room_off.png
  filter: brightness(100%)
  state_filter:
    'on': brightness(130%) saturate(1.5)
  style:
    ... 

Example using camera and service call:

- type: image
  entity: camera.demo_camera
  tap_action: call_service
  service: camera.snapshot
  service_data:
    filename: /tmp/snapshot
  style:
    ... 

@ghost ghost added the in progress label Jul 9, 2018
import '../../../components/buttons/ha-call-service-button.js';
import '../../../components/entity/ha-state-label-badge.js';
import '../../../components/entity/state-badge.js';
import '../components/hui-image.js';
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please order imports by depth. This is a local Lovelace import, but it at the bottom.

return html`
<style>
:host {
overflow-y: hidden;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If this is because of it being placed in entity-picture, the CSS should be added to entity-picture. We should never add CSS to web components because it helps styling it in a parent. The parent should contain the styling instead.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This became apparent because of picture-elements, but I think it is the correct behavior in general. If the parent has a static height I feel like the expectation is that the image won't overflow. But the only place this is a factor right now is picture-elements, so I'll move this css there and reassess if it comes up other places too

@c727
Copy link
Copy Markdown
Contributor

c727 commented Jul 9, 2018

The name should be state-image for consistency.

MAYBE we can also use one click handler for all elements

@balloob
Copy link
Copy Markdown
Member

balloob commented Jul 9, 2018

One click handler per element is a lot easier to reason about.

@jeradM
Copy link
Copy Markdown
Member Author

jeradM commented Jul 9, 2018

I added a separate one for images because images allow service calls where the others don't. We could allow service calls for the other element types too, or check the type in the handler if it's trying to call a service. I'll go with whatever solution you guys decide on though

@jeradM
Copy link
Copy Markdown
Member Author

jeradM commented Jul 9, 2018

Also, @c727 I didn't use state-image because I was concerned people would get confused if they weren't trying to setup state_images. I will change it if you'd rather keep the naming consistent for all elements though

@c727
Copy link
Copy Markdown
Contributor

c727 commented Jul 10, 2018

yep, I thought someone might also wants to use servicecalls on the other state-elements instead of toggle/dialog, but we can also change this later.

If I suggest changes I'm always open for discussion, not every suggestion must be good :P

@jeradM
Copy link
Copy Markdown
Member Author

jeradM commented Jul 10, 2018

You have done a lot more work on lovelace than me so I like to defer to your judgement, but you have to fight it out with balloob 😄

@balloob
Copy link
Copy Markdown
Member

balloob commented Jul 10, 2018

We can always add a helper method, _maybeAttachCallService(element, elementConfig)

@jeradM
Copy link
Copy Markdown
Member Author

jeradM commented Jul 10, 2018

@balloob are you talking about a helper to add a separate click handler? Something like https://hastebin.com/oyuyeyifox.javascript

@balloob
Copy link
Copy Markdown
Member

balloob commented Jul 11, 2018

Yep. That way we don't need to implement that method for every element and enforce the same config

@jeradM
Copy link
Copy Markdown
Member Author

jeradM commented Jul 12, 2018

I originally went with the helper, but it seemed really redundant. We need click handlers on 3 of the element types which are the same 3 that the helper would have been wrapping since the other 3 types handle clicks differently, so I just put the service call logic in the click handler. I will change it if you still think it would be better

@jeradM jeradM changed the title WIP: Add image type for picture-elements card. Add image type for picture-elements card. Jul 12, 2018
@balloob
Copy link
Copy Markdown
Member

balloob commented Jul 12, 2018

Looks good.

@balloob balloob merged commit 19e58c7 into home-assistant:master Jul 12, 2018
@ghost ghost removed the in progress label Jul 12, 2018
@jeradM jeradM deleted the image-elements branch July 14, 2018 01:25
@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.

4 participants