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

Use "limit" instead of "mode" for Numeric state trigger and condition #23360

Merged
merged 4 commits into from
Dec 22, 2024

Conversation

NoRi2909
Copy link
Contributor

@NoRi2909 NoRi2909 commented Dec 20, 2024

Proposed change

Currently the labels for "Above" and "Below" sections of the Numeric state trigger and condition are called "Above mode" and "Below mode":

image

As modes are always exclusive – you can't have two modes enabled at the same time – this labeling is misleading as you can specify both a lower and an upper limit.

By changing them to "Lower limit" (instead if "Above mode") and "Upper limit" (instead of "Below mode") it becomes clear that the user can specify either or both at the same time.

As the field labels remain "Above" and "Below" and the summary also explains it that way it remains clear the limits are exclusive.

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

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:

Currently the labels for "Above" and "Below" sections of the Numeric state trigger and condition are called "Above mode" and "Below mode".

As modes are always exclusive – you can't have two modes enabled at the same time – this labeling is misleading as you can specify both a lower and an upper limit.

By changing them to "Lower limit" and "Upper limit" it becomes clear that the user can specify either or both at the same time.

As the field labels remain "Above" and "Below" and the summary also explains it that way it remains clear the limits are exclusive.
@bramkragten
Copy link
Member

The label is actually just for the radio, but I get your point

Comment on lines 3169 to 3170
"mode_above": "Lower limit",
"mode_below": "Upper limit",
Copy link
Member

Choose a reason for hiding this comment

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

I think we should rename the key names if we are going to use them as a description for the entire section.

Copy link
Contributor Author

@NoRi2909 NoRi2909 Dec 22, 2024

Choose a reason for hiding this comment

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

Sure makes sense for consistency. Then we need to also rename them for the condition as this references the labels from the trigger:

"mode_above": "[%key:ui::panel::config::automation::editor::triggers::type::numeric_state::mode_above%]",
"mode_below": "[%key:ui::panel::config::automation::editor::triggers::type::numeric_state::mode_below%]",

Shall I go ahead with that?

Copy link
Member

Choose a reason for hiding this comment

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

Yes please 👍

Copy link
Contributor Author

@NoRi2909 NoRi2909 Dec 22, 2024

Choose a reason for hiding this comment

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

I made the additional changes in the strings.json but I'm afraid that the necessary changes in

ha-automation-trigger-numeric_state.ts
ha-automation-condition-numeric_state.ts

are beyond my rudimentary programming skills and what the GitHub web editor allows me to mess with …

Edit: I figured out my way round the editor and did a full find & replace on both. Now let's see …

@bramkragten bramkragten merged commit 8b63824 into home-assistant:dev Dec 22, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants