-
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
Add number tile feature #18562
Add number tile feature #18562
Conversation
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.
Thanks ! I added few comments.
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Co-authored-by: Paul Bottein <[email protected]>
private async _setValue(ev: CustomEvent) { | ||
const stateObj = this.stateObj!; | ||
|
||
await this.hass!.callService("input_number", "set_value", { |
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 think that for number domain the service that should be called is number.set_value
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.
Yes, I just added that domain so I hadn't tested it yet, good catch.
src/panels/lovelace/editor/config-elements/hui-tile-card-features-editor.ts
Show resolved
Hide resolved
private async _setValue(ev: CustomEvent) { | ||
const stateObj = this.stateObj!; | ||
|
||
const domain = computeDomain(stateObj.entity_id); |
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.
can we computeDomain once or memoize it? Now it is calculated every time we want to change the value.
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 don't think this will change something because computeDomain
is a very simple function and it will make the code complicated. Memoization must be used for large computing like transforming array or computing complex object.
Adding memoization here will be worst because it will introduce multiple checks to compute the value.
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.
This function also only runs on change not on render so it won't run that often.
@JosephAbbey is this ready to review? |
Thank you ! Ok for me ! |
Proposed change
Number Input (buttons or slider)
Type of change
Example configuration
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed: