-
Notifications
You must be signed in to change notification settings - Fork 52
[WC-2984] Events: Parameter type support #1771
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
base: main
Are you sure you want to change the base?
Conversation
…eter type support
… in delay and repeat properties
onEventChangeDelay: 0, | ||
componentLoadDelayParameterType: "number", | ||
componentLoadDelayInteger: 0, | ||
componentLoadDelayExpression: { value: undefined, status: ValueStatus.Unavailable }, |
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.
There is dynamicValue
helper for this, in the same place where actionValue
is coming from.
<description>Interval between repeat action execution. Value is in milliseconds.</description> | ||
</property> | ||
|
||
<property key="componentLoadRepeatExpression" type="expression" required="false"> |
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 originally property was called componentLoadRepeatInterval
(and should stay this way, old names can't be changed), isn't componentLoadRepeatIntervalExpression
is a better name?
Same for componentLoadRepeatParameterType
-> componentLoadRepeatIntervalParameterType
.
Previously we had componentLoadRepeatInterval
- componentLoad is prefix, forget about that for a moment. So we have
RepeatInterval
as number, and we need to extend the properties, so we need following properties:
(Repeat interval) Parameter Type (switch between types)
(Repeat interval) Static (for static number )
(Repeat interval) Expression (for expression)
So logical names would be
[prefix]RepeatIntervalParameterType
[prefix]RepeatIntervalStatic
[prefix]RepeatIntervalExpression
So far so good, but because static property already existed there under name [prefix]RepeatInterval
, we have to keep it, so even though it is a bit inconvenient naming we have to do the following:
[prefix]RepeatIntervalParameterType
[prefix]RepeatInterval
(old name, so that property value is not getting lost)
[prefix]RepeatIntervalExpression
Hope this makes sense.
Check other names and standardize the convention behind those names.
…ay and interval values
Pull request type
New feature (non-breaking change which adds functionality)
Description
When configuring the Events widget to run specific action on load or on attribute change, previously user only could set a static value when setting the delay value or repeat interval.
I enhanced this by adding a the possibility to set these value with expressions. This way users have more flexibility when configuring their events.
What should be covered while testing?