Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support icon_height for entity button #2800

Merged
merged 4 commits into from May 13, 2019
Merged

Support icon_height for entity button #2800

merged 4 commits into from May 13, 2019

Conversation

timmo001
Copy link
Contributor

@timmo001 timmo001 commented Feb 19, 2019

Description

Adds support for icon_height with the entity-button card. This allows the user to tweak the height of the icon in the UI. Also added to config UI and moved the icon and icon_height to a new line.

Screenshots

image

image

image

Checklist

  • Code is tested and works on my devices

@iantrich
Copy link
Member

iantrich commented Feb 19, 2019

I have no problem with this, I guess, but am planning on doing a big overhaul of this card in the near future which might break this new config option. My initial thought, related to this, is that icon will be an object, i.e.

icon:
  icon: mdi:home
  style:
    height: 100px
  state_filter:
    "on": brightness(110%) saturate(1.2)
    "off": brightness(50%) hue-rotate(45deg)

@balloob
Copy link
Member

balloob commented Feb 19, 2019

No breaking changes for Lovelace @iantrich. We're out in the wild and people with UI editors should not have to jump into YAML. You would need to come up with a proposal for config migrations first 😉

What I don't like about the icon height for users is that it's not clear that they can't just enter a number, but have to add 100px. I don't want to let CSS leak into our editor.

@iantrich
Copy link
Member

we have css in all our picture-element elements. it's a necessary evil. that being said, style should be an advanced option with appropriate warnings to users.

config migrations...yuck, but understandable...can we go back to beta?

@balloob
Copy link
Member

balloob commented Feb 19, 2019

For picture elements it's a necessary evil. I don't want that to be a reason to have something as simple as an entity button get overcomplicated.

For a UI editor of entity button, it should be an input with type=number>, and the code converts it to add px. That way it's user friendly.

However, we end up discussing all the internals without looking at the bigger picture: is this a feature that we want to add?

@iantrich
Copy link
Member

Just height, honestly, no.

As I mentioned before, I want to overhaul the entity-button and in doing so I planned on using the icon-element which supports style. Same would be true for the button itself using a refactored call-service button element. The idea being that the button element isn't really much different than the button card, except for wrapped in a card; same is true for a button row, but wrapped in a generic row.

The main point I want to emphasize though is that adding single limited tweaks is going to be a lot more work than just allowing a pass-through style object. It would be an advanced feature, hidden by default, and wouldn't add complexity to the element itself as we're already doing it, would just add to advanced features for the objects and would ease a lot of headaches users have.

I understand wanting to keep things simple, I still believe in that too, and in fact think from a code perspective we are still achieving that. But users want to customize, want to tweak beyond just themes, and I think the style object is the easiest way to do this. Hopefully I can convince you 😉

@timmo001 sorry for hi-jacking your PR 😄

@balloob
Copy link
Member

balloob commented Feb 19, 2019

I thought that for advanced crazy stuff, we would defer to custom things, and for our built-in cards stick to themes

@timmo001
Copy link
Contributor Author

So make the 100px be 100 for example and stitch on the px after the config value if I read correctly? Yeah CSS can be a bit of a rabbit hole that could cause issues for people on these 'standard' cards so I get it

@iantrich
Copy link
Member

I thought that for advanced crazy stuff, we would defer to custom things, and for our built-in cards stick to themes

custom things, like @thomasloven card-modder do this at the ha-card level but don't allow for icon customization. Regardless, for now, we can consider the discussion moot, and I'll plan on submitting a POC PR in the future showing the added benefits 😄

@timmo001
Copy link
Contributor Author

I've changed the icon_height to use numbers instead of allowing CSS to be used

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.

Looks good!

I think only this change, so our advanced users can still do their thing.

@@ -115,6 +115,9 @@ class HuiEntityButtonCard extends LitElement implements LovelaceCard {
style="${styleMap({
filter: this._computeBrightness(stateObj),
color: this._computeColor(stateObj),
height: this._config.icon_height
Copy link
Member

Choose a reason for hiding this comment

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

The config should contain a valid CSS value, so it should be stored with px already added to it. The UI editor should only support a value that ends in px and should allow editing it as a number, not exposing the px to the user. That way, advanced users can still use a % or other CSS values in their YAML.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not liking this idea. So I have to use the YAML editor in order to use %? And what happens if I use % via the raw config editor and then open the UI editor?

I don't think we should mess with the user's input. Just use what they give us. Perhaps we can do better validation in the future, but I don't think it's necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I'll add a suffix to the UI side and allow the yaml to edited as the advanced user likes then just pull in the setting as is on the card.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I seem to have it working.

To go on @iantrich 's point. I've managed to get it so it leaves this field alone but without messing with the 'advanced' input when you switch back to or save and reopen the config UI.

@timmo001
Copy link
Contributor Author

timmo001 commented May 10, 2019

OK, I think that's it. Took some finagling, but CSS is now supported with px used in the config UI.

testing

Excuse the long/poor quality testing gif 😄

@balloob
Copy link
Member

balloob commented May 13, 2019

🎉 🎉 🎉

@balloob balloob merged commit 309fecc into home-assistant:dev May 13, 2019
@timmo001 timmo001 deleted the support-icon-height-entity-button branch May 13, 2019 11:44
@balloob balloob mentioned this pull request May 14, 2019
@balloob
Copy link
Member

balloob commented May 14, 2019

Can you open a PR for the docs? I had to do a new release of the frontend to fix a bug, so we are shipping this feature tomorrow 😉

@timmo001
Copy link
Contributor Author

Can you open a PR for the docs? I had to do a new release of the frontend to fix a bug, so we are shipping this feature tomorrow 😉

Here you go 😺 home-assistant/home-assistant.io#9449

balloob pushed a commit to home-assistant/home-assistant.io that referenced this pull request May 14, 2019
* ✨ Add entity button icon_height docs

Further to home-assistant/frontend#2800 (comment)

* 🔨 Expand
balloob pushed a commit to home-assistant/home-assistant.io that referenced this pull request May 14, 2019
* ✨ Add entity button icon_height docs

Further to home-assistant/frontend#2800 (comment)

* 🔨 Expand
@keesschollaart81 keesschollaart81 mentioned this pull request May 19, 2019
@github-actions github-actions bot locked and limited conversation to collaborators Jul 6, 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.

None yet

4 participants