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

New warning row for non-existent entities #1946

Merged
merged 3 commits into from
Nov 2, 2018

Conversation

iantrich
Copy link
Member

@iantrich iantrich commented Nov 1, 2018

No description provided.

@ghost ghost assigned iantrich Nov 1, 2018
@ghost ghost added the in progress label Nov 1, 2018
}

protected render(): TemplateResult {
if (!this.entity) {
Copy link
Member

Choose a reason for hiding this comment

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

I think that we should always render this element, even if no entity set. Make it ${this.enttiy || ''} below and it's fine

Copy link
Member

Choose a reason for hiding this comment

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

I also indirectly meant that we should remove this if, or else it doesn't make sense to default below.

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 comment addressed.

private renderStyle(): TemplateResult {
return html`
<style>
div {
Copy link
Member

Choose a reason for hiding this comment

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

Oh, one more comment. Skip the whole div. Remove it from the template and update CSS to style :host

:host {
  display: block;
  background-color: yellow;
  padding: 8px;
}

btw do we know why we use flex: 1?

Copy link
Member Author

Choose a reason for hiding this comment

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

I do not

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe I pulled that from somewhere and just kept the copypasta without checking any of it since it just worked. (Hearing that out loud sounds like I need to put the keyboard up smh)

Copy link
Member Author

Choose a reason for hiding this comment

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

That's not something I would ever do...

Copy link
Member

Choose a reason for hiding this comment

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

I can see that we only work with the best engineers here 👍

Switch to :host was good, I removed the <div> as it is no longer needed.

Copy link
Member Author

@iantrich iantrich Nov 2, 2018

Choose a reason for hiding this comment

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

Hey, you brought on a C++ dev that started front-end stuff three months ago 🤣
I am very much the "well it works" crowd right now, but am trying to learn the why as well.

@ghost ghost assigned balloob Nov 2, 2018
@balloob balloob merged commit 6cc67dc into home-assistant:dev Nov 2, 2018
@ghost ghost removed the in progress label Nov 2, 2018
@iantrich iantrich deleted the error-row branch November 16, 2018 19:18
@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