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

Conditional element support for use in picture-elements #2865

Merged
merged 9 commits into from Mar 3, 2019

Conversation

yosilevy
Copy link
Contributor

(Re-posting as standalone code + joint utility functions)

This was requested a while back by a bunch of people. This can allow to take picture-elements in to the next level (I think this is needed in many other cards as well.
For example it can help create an interactive multi device remote. Now people just show/hide whole cards and that's bad UX...

Here's just a simple demo of just one control:
https://www.dropbox.com/s/h6t2hlrbj07qs94/Capture.mp4?dl=0

Need to update docs and I can also make a nice video to post in the docs

@@ -76,3 +76,9 @@ export const saveConfig = (
type: "lovelace/config/save",
config,
});

export interface Condition {
Copy link
Member

Choose a reason for hiding this comment

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

Move this to lovelace/common/validate-condition.ts

elements: LovelaceElementConfig[];
}

class HuiConditionalElement extends LitElement implements LovelaceElement {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this a LitElement and not HTMLElement like the conditional card?

return html`
<style>
:host {
position: absolute;
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 extend HTMLElement and not use shadow DOM, this probably won't be necessary, the element will be able to position itself ?

Having it position absolute means it will overlay all buttons and make those not clickable ?

`;
}

private _createHuiElement(
Copy link
Member

Choose a reason for hiding this comment

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

Can we extract this into a method that is shared with the picture elements card?

}

return html`
${this.renderStyle()}
Copy link
Member

Choose a reason for hiding this comment

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

Even if you are going to use Lit (which you shouldn't), don't use renderStyle() but instead use static get styles

@yosilevy
Copy link
Contributor Author

yosilevy commented Mar 1, 2019

Resolved all of the above and simplified. Had to use PolymerElement since I need to know when I'm ready because now the elements go in to the parent to avoid styling and click shadowing. Therefore, I need to know when I have a parent. Comments welcome.

@iantrich
Copy link
Member

iantrich commented Mar 1, 2019

We are moving away from polymerelement to litelement

@yosilevy
Copy link
Contributor Author

yosilevy commented Mar 1, 2019

I know but @balloob wanted it to be HTMLElement but the problem is I need to be able to know when it's ready so I have a parent to set the elements in to (to avoid styling and click shadowing) so I ended up using PolymerElement to get the event. I can switch to Lit or if you have another solution to stay with HTMLElement that would be good. @balloob ?

@iantrich
Copy link
Member

iantrich commented Mar 1, 2019

I don't know about an HTMLElement solution, but LitElement is the replacement for PolymerElement, so we should always prefer LitElement if it is truly needed. Wait to hear back from @balloob before refactoring though.

@@ -1,6 +1,6 @@
import { html, LitElement, TemplateResult } from "lit-element";

import { createHuiElement } from "../common/create-hui-element";
import { createConfiguredHuiElement } from "../../lovelace/common/validate-condition";
Copy link
Member

Choose a reason for hiding this comment

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

This method should obviously not be in common or in validate-condition. It's unique to this card and the conditional card.

state_not?: string;
}

export function createConfiguredHuiElement(
Copy link
Member

Choose a reason for hiding this comment

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

This has nothing to do with this file.

@@ -0,0 +1,83 @@
import { PolymerElement } from "@polymer/polymer/polymer-element";
Copy link
Member

Choose a reason for hiding this comment

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

Don't create Polymer.

});
}

public ready() {
Copy link
Member

Choose a reason for hiding this comment

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

connectedCallback / disconnectedCallback are part of the custom element spec, also available on HTMLElement. Use that.

@yosilevy
Copy link
Contributor Author

yosilevy commented Mar 1, 2019

All done. Thanks for the review. I thought about connected callback but VS code intellisense threw me off.

this._config = config;

this._config.elements.map((elementConfig: LovelaceElementConfig) => {
this._elements.push(
Copy link
Member

Choose a reason for hiding this comment

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

Why is this an array? You are only ever creating a single element.

Copy link
Member

Choose a reason for hiding this comment

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

Ah see it now. that's fine.

You are still not clearing this._elements and cleaning up old elements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We support multiple elements under the same set of conditions


this._config.elements.map((elementConfig: LovelaceElementConfig) => {
this._elements.push(
createConfiguredHuiElement(elementConfig, this._hass!)
Copy link
Member

Choose a reason for hiding this comment

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

You don't have this._hass here. We always call setConfig first.

Copy link
Member

Choose a reason for hiding this comment

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

createConfiguredHuiElement should not take hass as that's not needed for creation of the 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.

It does need hass to provide it to the element. This is exactly what the local _createHuiElement method used to do and it is required (not for creation but for full operation).

});

if (this._hass) {
this.hass = this._hass;
Copy link
Member

Choose a reason for hiding this comment

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

You're doing this here and in connectedCallback. It's confusing. I think that we should instead just have a render method that does the right thing ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's needed because config can be called even after component was fully loaded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And render won't help since we're adding the elements to the parent to maintain positioning as regular picture-elements elements...

throw new Error("Error in card configuration.");
}

if (this._elements && this._elements.length > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

No need to check for this._elements, it is initialized as []

}

private update() {
if (this._hass) {
Copy link
Member

Choose a reason for hiding this comment

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

haha, I meant to put the visible check and appending elements in here 😆

@@ -75,6 +64,12 @@ class HuiConditionalCard extends HTMLElement implements LovelaceCard {
public getCardSize() {
return computeCardSize(this._card!);
}

private update() {
Copy link
Member

Choose a reason for hiding this comment

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

This is silly. Move EVERYTHING from the hass setter, except for assigning this._hass = hass, into this method. So that the update method is responsible for updates.

Copy link
Member

Choose a reason for hiding this comment

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

Just like updateElements for conditional 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.

Yes I forgot picture-elements. Done.

@balloob balloob merged commit 19c44f7 into home-assistant:dev Mar 3, 2019
@ghost ghost removed the in progress label Mar 3, 2019
@balloob
Copy link
Member

balloob commented Mar 3, 2019

🎉 🎉 🎉 🎉

@balloob
Copy link
Member

balloob commented Mar 3, 2019

Please open a PR for the docs repo

@yosilevy
Copy link
Contributor Author

yosilevy commented Mar 3, 2019

Already open - home-assistant/home-assistant.io#8795

@balloob balloob mentioned this pull request Mar 4, 2019
@yosilevy yosilevy deleted the conditional-element-support branch March 5, 2019 19:05
@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.

None yet

4 participants