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

Add selectors to ha-form #11534

Merged
merged 4 commits into from Feb 4, 2022
Merged

Add selectors to ha-form #11534

merged 4 commits into from Feb 4, 2022

Conversation

balloob
Copy link
Member

@balloob balloob commented Feb 3, 2022

Breaking change

Proposed change

Add support for selectors in ha-form.

Goal is to eventually remove all other types and be selector only.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue or discussion:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

@balloob balloob added the needs design preview PRs with this label will trigger a GitHub action to generate a gallery preview label Feb 3, 2022
zsarnett
zsarnett previously approved these changes Feb 3, 2022
bramkragten
bramkragten previously approved these changes Feb 3, 2022
Comment on lines 20 to 21
required?: boolean;
optional?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

We should decided which one to use by default, maybe drop 1 from the type

Copy link
Member

Choose a reason for hiding this comment

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

I would say required

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, but let's do that in a future PR. We will need to check what is used where.

For the backend, both required and optional fields can have a default, which is filled in when key is omitted. So not so required or optional…

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 agree with keeping required. Let's update it in another PR. It means rewriting all checks for optional to be !required.

Copy link
Member Author

Choose a reason for hiding this comment

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

@balloob balloob dismissed stale reviews from bramkragten and zsarnett via 442533b February 3, 2022 22:35
@balloob
Copy link
Member Author

balloob commented Feb 3, 2022

Don't understand why the error would happen

image

@balloob
Copy link
Member Author

balloob commented Feb 3, 2022

I bet this has something to do with icons being imported by the supervisor now that we import ha-selector. I bet @bramkragten knows the answer when he wakes up

@bramkragten
Copy link
Member

Yep, that's the issue

@balloob
Copy link
Member Author

balloob commented Feb 4, 2022

selector-entity -> ha-entity-picker -> state-badge -> ha-state-icon -> ha-icon -> 💣

@balloob
Copy link
Member Author

balloob commented Feb 4, 2022

I've fixed it now by blocking ha-icon from being loaded on supervisor builds. It wasn't used since it broke the build. Not sure if this is ok or we should find a solution where we break up selector in a "without icon" version safe for the supervisor and a "with icon" version for HA?

Comment on lines +50 to +51
{ name: "text", selector: { text: { multiline: false } } },
{ name: "text_multiline", selector: { text: { multiline: true } } },
Copy link
Member

Choose a reason for hiding this comment

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

Forgive me for my ignorance/lack of knowledge on this part.

Can't we handle the selector options?
As in, would be nice if multiline was not defined by the schema name.

The same issue also returns in, for example, the entity selector. Would be nice if we could pass in data from the flow limiting, e.g., the domain you could select.

Copy link
Member

Choose a reason for hiding this comment

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

That is the case, the name is just for the demo. These are the same selectors that are used in blueprints and service calls.

@bramkragten
Copy link
Member

I think this is ok. ha-icon should never be used in hassio. Only downside is that we no longer have an indication that there is a ha-icon in the build.

@bramkragten bramkragten merged commit 0046252 into dev Feb 4, 2022
@bramkragten bramkragten deleted the ha-form-selectors branch February 4, 2022 11:47
@github-actions github-actions bot locked and limited conversation to collaborators Feb 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed needs design preview PRs with this label will trigger a GitHub action to generate a gallery preview
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants