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

feat: add visual editor #1128

Open
wants to merge 26 commits into
base: dev
Choose a base branch
from
Open

feat: add visual editor #1128

wants to merge 26 commits into from

Conversation

selvalt7
Copy link

Added translations
Fixed not able to add entity in a new card
Added option whether to use color thresholds
Added headers to sub pages
Css adjustments
Added more settings
Entity form now uses grid for selectors positioning
Added clear button to color selector
Color thresholds are in expansion panels now
Added support for entity as value
Added subpage header and subpage link elements
Organized files
Added few default values
Added bounds and value factor options
Color selector small css tweak
Empty color selector is now indicated by a checkerboard
Removed support for color rgb array
@akloeckner
Copy link
Collaborator

Wow! Very impressive. I will need some time to dive into this. And I will most likely need a few iterations until I understand everything.

If you appreciate some early feedback from my browsing around for the first time:

  • I found this glitch in the "Display" submenu, which does not happen in the "Alignment" submenu:
    image

  • We are having some issues with the package-lock.json file. It loses resolved and integrity fields, whenever a new package is installed. You also ran into this issue. The only fix I could find so far, was to manually select the changes to commit and revert all the others.

  • The entities show two buttons to delete them:
    image
    Maybe we can manage to show and use the default one only?

@selvalt7
Copy link
Author

Made some changes based on your feedback

  • Glitch was caused because select selector does not like options with boolean value, the fix made selector accept custom value.
  • As for the remove entity button, I removed it and made one inside the selector act as the external.
    entity_selector

@ildar170975
Copy link
Collaborator

Just a small remark:
For some STOCK cards a UI does not provide controls for changing some options - i.e. these options may only be changed in yaml. There are some related issues in HA frontend Github - and some of these issues are closed be Devs as "well, it turned to be normal to force users to use yaml to define some options", some issues may be just ignored.
Means - may be we should not try to support ALL available options. Besides, one day some options may become per-entity - and then changes in UI will be needed.
I am really satisfied that @selvalt7 started developing UI - but I do not insist on a support of ALL available options.

@ildar170975
Copy link
Collaborator

Another remark is about selecting entities.
Since I have not tested is PR - have to assume & guess about a possible use of "common HA ways" in this PR.
The current "common way" of selecting entity - and then displaying a selected entity - is rather confusing & unfriendly:

  1. After being selected - an entity is shown by friendly_name which can be very confusing since names are not unique. Check Can't recognise entities only by its names in the UI automation editor home-assistant/frontend#11870 for Automations UI; similarly it is about editing Entities card (& all similar places in HA).
  2. When a drop-down UI control is reopened - the selected entity is not "selected" (Selected entity not highlighted in entity picker home-assistant/frontend#17395) - which makes hard to guess which entity is selected (unless you open a yaml).

Means - using same "common ways" just "contaminate" more cards in HA ))).
Not a big deal, using yaml anyway - just a warning.

@selvalt7
Copy link
Author

selvalt7 commented Aug 22, 2024

Visual editor does not provide all the options, only the ones I felt are necessary, if user wants to change card size or change update interval to something other than every state change, it can only be done with yaml.

On the subject of selected entity which is represented with friendly_name. I came up with an idea to display entity_id under the selector like this
entity selector

@ildar170975
Copy link
Collaborator

I came up with an idea to display entity_id under the selector like this

Good, very useful & makes a picture clear.

And what about this common issue?
I was afraid that you will face same glitch if decide to use a common component.

@selvalt7
Copy link
Author

Yeah I have that issue, the only solution I think is to make custom selector or wait until they fix it in entity selector.

This should help identify selected entity as the friendly_name is not unique and the default entity selector does not correctly highlight selected entity
@ildar170975
Copy link
Collaborator

wait until they fix it in entity selector.

I would choose this way for time-saving.

Copy link
Collaborator

@akloeckner akloeckner left a comment

Choose a reason for hiding this comment

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

I finally got around to dig into some of the details of your (still impressive!) PR. Thanks for hanging on. I might have more feedback later, but I thought I'd better share it sooner than later.

To sum up my thoughts up front in a few points: I found one or two minor glitches. And I think we can improve some of the code by re-using common patterns and facilities provided by hass itself, which may also help to increase similarity to other cards' config editors.

Please have a look and let me know what you think. I might also not have grasped the full picture yet.

align_state: 'left',
smoothing: true,
...this._config,
show: { ...DEFAULT_SHOW, ...this._config.show },
Copy link
Collaborator

Choose a reason for hiding this comment

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

I noticed, the generated config is somehow more involved than the minimal config I started with. I suspect, it has to do with merging the defaults into the generated configuration.

I also noticed that I cannot clear fields such as the show.points = hover option. This could be related.

I propose that default options should not be added explicitly to the configuration, if they are not selected explicitly by the user. (That is, DEFAULT_SHOW and also the other options above.)


if (this.subElementEditorConfig !== undefined) {
return this.renderSubElement();
// return html`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this comment is a left-over that should be removed?

],
},
{
name: 'tap_action',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add this to the "Appearance" group? It appears a bit lost in the editor.

const SCHEMA = [
{
name: '',
type: 'expandable',
Copy link
Collaborator

Choose a reason for hiding this comment

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

At first, I struggled a bit with the sub-page editor, because I had never seen it in HA before. Whereas I have seen these expandable groups before. But I understand now that the expandble groups can't meanigfully be added to the entity list. So, I agree with the sub-pages solution.

Maybe it will help tidying up the UI, if we also move this group to a sub-page? Possibly even move all groups to sub-pages to keep it consistent?

import { DEFAULT_SHOW } from '../const';
import { localize } from '../localize/localize';

const SCHEMA = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it make sense to put these long jsons in an editor/const.js?

.hass=${this.hass}
.data=${data}
.schema=${SCHEMA}
.computeLabel=${this.computeLabel}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make sense to directly pass localize here? And move all the logic to localize()?

This would then allow to also fill .localizeValue, in order to localize the values, too.


computeLabel(schema) {
const localized = this.hass.localize(`ui.panel.lovelace.editor.card.generic.${schema.name}`);
if (localized === '') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any downside in just returning this.hass.localize(...) || localize(...)?

If not, it seems to be common to omit the elseand do

if (localized !== '') {
  return localized;
}
return localize(...);

hex_color: {},
};

export class CustomColorSelector extends LitElement {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason not to use the color picker provided by hass itself?

If it is the "clearable" feature, it could be handled in the usual hass way by having an enable/disable toggle in front of the color picker.

If it is the hex format, could it be converted "output-only" in the main valueChanged function? I am not familiar enough with the way these things work...

Copy link
Author

Choose a reason for hiding this comment

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

Mainly it was the hex format. I've tried to do the convertion on output but this resulted in warnings in console about supplied color value to hass color picker but the selecter was rendering just fine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I see... I'd guess, we'd have to convert back and forth to not loose colours on each startup of the editor... At least in setConfig, configChanged and entitiesChanged... Then use [r,g,b] internally in the editor and hex outside...

Another idea: How about we allow [r,g,b] as a new colour input format for the mini-graph-card itself? Better suited for a follow-up PR then, maybe... We'd still loose non-hex colours on first startup of the editor, I assume...

I shall test, if losing colours is actually an issue...

Copy link
Collaborator

Choose a reason for hiding this comment

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

I did a test, and yes, loosing colours is an issue. Try this:

  • Edit card
  • Switch to YAML
  • Set colour to "yellow" on an entity
  • Switch to UI mode and edit entity
  • Observe the colour to be black now.

Copy link
Author

Choose a reason for hiding this comment

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

I've addressed this issue in the latest commit

if (!this._config || !this.hass) {
return;
}
// this._config = { config, ...this._entities };
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a left-over comment?

src/main.js Outdated
return {
entities: [],
color_thresholds: [],
show: DEFAULT_SHOW,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not understand, when this is used. If it automatically adds the default show options to each new card, we should not do this, I think. If it just can be used by the user to get a sensible template to fill in, I think it is a good idea to return all these otions here...

Copy link
Author

Choose a reason for hiding this comment

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

This was needed because things like adding new entity on a new card was now working properly but I sorted it out internally so it is not needed anymore and will be deleted.

Color thresholds and state map now use the new `ha-form-mgc-list`
Removed old unused components
Moved schemas to `editorConst.js`
Translations are now handled through `hass.localize`
Color selector should now display proper color when supplied with color name
Fixed inability to clear few options
Removed leftover comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants