-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Support for disabled selectors in config flow #22592
base: dev
Are you sure you want to change the base?
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
src/components/ha-form/types.ts
Outdated
@@ -25,6 +25,8 @@ export interface HaFormBaseSchema { | |||
suffix?: string; | |||
// This value will be set initially when form is loaded | |||
suggested_value?: HaFormData; | |||
// Readonly flag is passed here from backend for config flow | |||
readonly?: boolean; |
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.
nitpick: isn't read only 2 words? And so it should be read_only
and readOnly
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.
You're technically correct. readonly
is pretty established in frontend libraries though. We won't use it directly and convert it to disabled
so whatever core decides as long as it isn't disabled
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.
https://learn.microsoft.com/en-us/style-guide/a-z-word-list-term-collections/r/read-only gives read-only
which seems to also be in the dictionaries.
We should probably change to read_only
which translates to read-only
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 can change the var to read_only
, but I'm confused by what you mean by "translates to read-only
"
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 meant for end users it should say "read-only".
I didn't look at the frontend PR really but is there a tooltip or something for the end-users to understand these are read-only on purpose?
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.
As far as I know there is not a tooltip.
Maybe custom description strings could be used for the options flow fields if desired, right now they are reusing the description strings from the config flow.
I thought another enhancement could be to stick these fields in an expansion panel with a description of why they are there, but reading how config/options flow is handled in history stats I couldn't yet figure out how to make that work, so punting on that for now.
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.
Maybe custom description strings could be used for the options flow fields if desired, right now they are reusing the description strings from the config flow.
Probably the best way to handle it right now.
I thought another enhancement could be to stick these fields in an expansion panel with a description of why they are there, but reading how config/options flow is handled in history stats I couldn't yet figure out how to make that work, so punting on that for now.
Probably a section with a description would be quite nice. But sections are not really built in that way to only be used in the options flow as it's saving the data with the section.
… disabled-fields-config-flow
Proposed change
Support for home-assistant/core#129456
Type of change
Example configuration
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed: