-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Allow device triggers to specify extra fields, update ha-form with support for paper-time-input #3721
Allow device triggers to specify extra fields, update ha-form with support for paper-time-input #3721
Conversation
for
to device automation triggersfor
to device automation triggers
src/components/paper-time-input.js
Outdated
@@ -49,6 +49,7 @@ export class PaperTimeInput extends PolymerElement { | |||
display: none; | |||
} | |||
--paper-input-container-shared-input-style_-_-webkit-appearance: textfield; | |||
--paper-input-container-invalid-color: red; |
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 this styling should not be here?
Also, how to block saving the automation if the paper-time-input is invalid?
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 could move it to ha-style.ts
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.
OK, but how to add it such that it only applies to paper-input inside paper-time-input?
Can we add a days field? For if you want more than 23 hours? |
OK, makes sense, will give it a try. |
Yeah, I think it's fine to make the hours field take more than 23 with an option too |
Set display of the paper-input to inline-block and give it a proper width: paper-input {
display: inline-block;
width: 50px;
} |
Btw, the backend does seem to support days: # If given, will trigger when condition has been for X time, can also use days and milliseconds.
for:
hours: 1
minutes: 10
seconds: 5 https://www.home-assistant.io/docs/automation/trigger/#numeric-state-trigger |
@bramkragten Thanks for help with styling! Days and hh:mm:ss is now on a single row, but it's quite ugly |
We might be better of creating a new element for picking time that has all the logic you want and includes days, that could also handle the hours -> days conversion and export it in the right format. We could reuse that element for all automation triggers. Trying to get this component to do 2 totally different thing will be messy. Or we should add the days to the current time picker... |
this.hass = hass; | ||
if (!this.state.capabilities && trigger.domain) { | ||
fetchDeviceTriggerCapabilities(this.hass, trigger).then( | ||
(capabilities) => { | ||
this.setState({ ...this.state, capabilities }); | ||
} | ||
); | ||
} |
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.
fetchDeviceTriggerCapabilities
is called when device trigger is picked, but if we're editing an existing automation that won't happen. The purpose of this is just to load the trigger capabilities once.
OK to take care of it here, or should it be done elsewhere, for example in componentDidMount
?
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.
The render method should never have side effects. Not in Preact nor Lit. It is possible that this method can get called multiple times before the capabilities resolve, and you will do many requests.
Instead, do it inside componentDidUpdate
. (similar to updated
in Lit)
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.
OK, moved out of render but to componentDidMount
, componentDidUpdate
was not called after first render.
for
to device automation triggersb186b16
to
9ab126b
Compare
Can we drop days ? People can just do 72 hours if they want that. |
Already did |
Allow device triggers to specify extra fields which will then be shown in the UI using ha-form.
Some extra love is given to support
for
(duration) in device automation triggersScreenshot:
Intended for backend PRs home-assistant/core#26658, home-assistant/core#27133 etc.