-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Added migrate dialog when card has no ID #2008
Conversation
@@ -0,0 +1,83 @@ | |||
import { html, LitElement, PropertyDeclarations } from "@polymer/lit-element"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we look into adding a Lovelace/editor Localize for stuff like this? Usually not full sentences in the translation file but this is a pretty descriptive and useful message that english may not be the best hardcoded route? Just a thought
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Fixes: #1954 |
I think that this should not be it's own dialog, instead I suggest it becomes part of the Inside dialog-edit-card:
|
Thinking now, we only really need a "load yaml from backend" for the yaml editor, other editors can just use the JS config. |
src/panels/lovelace/common/data.ts
Outdated
@@ -12,10 +13,17 @@ export const getCardConfig = ( | |||
export const updateCardConfig = ( | |||
hass: HomeAssistant, | |||
cardId: string, | |||
config: any | |||
config: string | LovelaceConfig, | |||
configFormat: "js" | "yaml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
json. It's either json or yaml.
src/panels/lovelace/hui-view.js
Outdated
@@ -147,7 +147,8 @@ class HUIView extends PolymerElement { | |||
|
|||
const wrapper = document.createElement("hui-card-options"); | |||
wrapper.hass = this.hass; | |||
wrapper.cardId = cardConfig.id; | |||
wrapper.cardId = cardConfig.id ? String(cardConfig.id) : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No reason to set this, it's part of the cardConfig
@@ -28,10 +41,16 @@ export class HuiYAMLEditor extends LitElement { | |||
`; | |||
} | |||
|
|||
private async _loadConfig(): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! This was one of the confusing things. Perfect.
} | ||
} | ||
|
||
constructor() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just remove this?
|
||
set hass(value: HomeAssistant) { | ||
this._hass = value; | ||
if (!this.yaml || this.yaml === "") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't do it here, check in updated()
if cardId has changed or not. If it has, fetch yaml.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean shouldUpdate
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Only the lifecycle event updated
is to be used for side effects. Side effects are things that mutate state, or start things that will mutate state (like fetching data)
import { hassLocalizeLitMixin } from "../../../mixins/lit-localize-mixin"; | ||
import { migrateConfig } from "../common/data"; | ||
|
||
export class HuiDialogMigrate extends hassLocalizeLitMixin(LitElement) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not being used?
}; | ||
} | ||
|
||
public async showDialog({ hass, reloadLovelace }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should change how we do this, and instead store the params as 1 object instead of many props.
public async showDialog(dialogParams: MigrateCardDialogParams) {
this._dialogParams = dialogParams
I merged Zacs Pr, got merge conflicts now. |
return { | ||
hass: {}, | ||
}; | ||
return { hass: {} }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why this was changed. I dont think this is "prettier"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is, small objects will be put on a single line.
this._createCard(configValue); | ||
} | ||
|
||
set value(configValue: LovelaceConfig) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can make this set config
right? To align with everything else as well as its param type
private _originalConfigYaml?: string; | ||
private _configElement?: LovelaceCardEditor | null; | ||
export class HuiDialogEditCard extends hassLocalizeLitMixin(LitElement) { | ||
protected _hass?: HomeAssistant; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we making this a _
variable when its not private?
? html` | ||
<paper-button | ||
@click=" | ||
${this._toggle}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesnt seem "prettier". As well as the reset of them down this section
} | ||
</style> | ||
<paper-dialog with-backdrop> | ||
<h2>Card Configuration</h2> | ||
<h2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is becoming quite messy because we're having 3 times a turnery statement on the exact same logic. To make it easier, just extract those into methods.
Preferably I would want to put it in a new custom element but that probably won't work well with the CSS of paper-dialog
.
<paper-dialog with-backdrop>
${this._cardConfig!.id ? this._renderEditor() : this._renderMigrator()}
</paper-dialog>
src/translations/en.json
Outdated
@@ -936,6 +936,19 @@ | |||
"required_fields": "Fill in all required fields" | |||
} | |||
} | |||
}, | |||
"lovelace": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There already is a lovelace category, you should merge your changes into that one.
src/translations/en.json
Outdated
}, | ||
"lovelace": { | ||
"editor": { | ||
"header": "Card Configuration", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels weird that one dialog is "top level" of editor and one below it. I think both should be below it: edit.header
and migrate.header
} | ||
|
||
private get _previewEl(): HuiYAMLCardPreview { | ||
return this.shadowRoot!.querySelector("hui-yaml-card-preview")!; | ||
public async showDialog({ hass, cardConfig, reloadLovelace }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a future PR, we should just make this a single object, not explode it, and type it. That way we can export the type and make sure people call it correctly, even potentially have a helper.
public async showDialog(params: EditDialogParams) {
and then
export const showEditDialog = (element: HTMLElement, hass: HomeAssistant, cardConfig: CardConfig, reloadLoveLace: () => void) =>
fireEvent(element, 'show-edit-card-dialog', { hass, cardConfig, reloadLovelace })
this._cardConfig = cardConfig; | ||
this._reloadLovelace = reloadLovelace; | ||
await this.updateComplete; | ||
this._cardConfig!.id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.shadowRoot.children[0].showDialog()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can typescript handle that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(this.shadowRoot.children[0] as any).showDialog()
|
||
public async openDialog(): Promise<void> { | ||
// Wait till dialog is rendered. | ||
await this.updateComplete; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only have to do this if this._dialog
== null ?
|
||
protected render(): TemplateResult { | ||
return html` | ||
<style> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer having the style being included via renderStyle
?active="${this._loading}" | ||
alt="Loading" | ||
class="center" | ||
style="margin-bottom: 24px;" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use stylesheet
style="margin-bottom: 24px;" | ||
></paper-spinner> | ||
<paper-dialog-scrollable | ||
class="${classMap({ hidden: this._loading! })}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we hide this with CSS or just not render the whole element ?
when( | ||
this._configElement, | ||
() => this._configElement, | ||
() => html` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that you can just use the string "loading…"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only important that the template returned from render
is wrapped in html
<hui-card-preview .hass="${this._hass}"></hui-card-preview> | ||
</paper-dialog-scrollable> | ||
<div | ||
class="paper-dialog-buttons ${classMap({ hidden: this._loading! })}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class="${classMap(…)}"
? but maybe also not render it when we're loading ?
this._configElement.setConfig(this._configValue!.value as LovelaceConfig); | ||
this._uiEditor = !this._uiEditor; | ||
} | ||
this._resizeDialog(); |
There was a problem hiding this comment.
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 only do this after await this.updateComplete
?
this._configValue!.format === "yaml" | ||
? yaml.safeDump(this._configValue!.value) | ||
: this._configValue!.value; | ||
if (JSON.stringify(configValue) === JSON.stringify(this._originalConfig)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return JSON.stringify(configValue) !== JSON.stringify(this._originalConfig)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. A couple of minor comments but this PR is pretty much done.
Fixes: #1954