Add getElementConfig to Glance + Add Form UI for updating YAML#1944
Add getElementConfig to Glance + Add Form UI for updating YAML#1944
Conversation
ghost
assigned
Current Progress |
zsarnett
force-pushed
the
config-element-glance
branch
from
4b537f8 to
3ac23ce
Compare
| } | ||
|
|
||
| private _handleConfigChanged(value: LovelaceConfig): void { | ||
| this._currentConfigYaml = yaml.safeDump(value); |
There was a problem hiding this comment.
I already wrote this in my previous comment and I will not allow this. This is SUPER INEFFICIENT. Every. single. key. stroke. will now result in a whole object being converted to YAML for no reason. To make it worse, it is converted back to JS objects inside the preview element.
We should only convert it to YAML when the users presses save. This PR cannot be merged with any other logic.
There was a problem hiding this comment.
We *should* convert every change to YAML. Do less and be lazy: we only need it as YAML when the user presses thet save button. We should store the exact same format as that we pass the preview element.
Sorry I was a bit confused... I should have asked for clarification. Will make the change in the PR
|
Yaml is now not updated each time the UI Editor is changed, Only updated when you change the yaml (direct Update), toggle to Yaml Editor (pulls in preview config) and when the user clicks save. |
zsarnett
changed the title
|
|
||
| private _handleYamlChanged(ev: YamlChangedEvent): void { | ||
| this._updatePreview(ev.detail.yaml); | ||
| this._updatePreview(yaml.safeLoad(ev.detail.yaml)); |
There was a problem hiding this comment.
This means that we no longer catch YAML parse errors and render a card with invalid YAML. I think that we should have a config value that contains format and value and have the preview element deal with that.
| } | ||
|
|
||
| private async _updateConfig(): Promise<void> { | ||
| this._currentConfigYaml = yaml.safeDump(this._previewEl.config); |
There was a problem hiding this comment.
It seems weird that we use our preview element as the source of truth of what the current config is.
| this._config = config; | ||
| } | ||
|
|
||
| get config() { |
There was a problem hiding this comment.
If we would want this, we could just store it as this.config directly, however I think that the use case is wrong and we should keep the value to save in the dialog editor card.
There was a problem hiding this comment.
The reason for this is that it tightly couples the implementation of the preview card with the edit dialog and limits reusability of the preview card.
| ); | ||
| this._configValue = { format: "js", value: conf }; | ||
| this._configElement = configElement; | ||
| } catch (err) { |
There was a problem hiding this comment.
Which things can raise here? We should make sure we only wrap those in our try … catch
There was a problem hiding this comment.
catch (err) {
if (!(err instanceof TypeError)) {
// tslint:disable-next-line:no-console
console.error(err);
}
this._configElement = null;
}
Something like this so we can log the errors we are unexpected?
There was a problem hiding this comment.
we shouldn't wrap anything that can raise a type error.
Whenever you see try…catch, we need to know exactly why the code inside can fail and guard for those clauses. I think in this case just the fetching of config element is something we want to guard for ?
There was a problem hiding this comment.
ah gotcha. I missunderstood.
| import computeStateName from "../../../common/entity/compute_state_name"; | ||
| import processConfigEntities from "../common/process-config-entities"; | ||
| import applyThemesOnElement from "../../../common/dom/apply_themes_on_element"; | ||
| import { fireEvent } from "../../../common/dom/fire_event.js"; |
There was a problem hiding this comment.
Just as a future note; these cleanups should be done in a separate PR. Each time I review this PR (and it's been a few times!), I see all these changes that are actually just noise of the actual code/features.
|
🎉 🎉 🎉 🎉 🎉 🎉 🎉 |
3 participants
This is a work in progress and my next step is creating Entity Element. That shows a List Box of entities, Delete Entity button, show options button that then gives options for entities (icon, name, etc)
Creating the PR now so that the implementation can be critiqued. I am going to work on the different Elements for now. Just want to know if I am going about this the correct way.
Fixes #1951
Fixes #1952
Fixes #1953