port: add handledProperty to emitEvent #3686
Conversation
| case 'bubbleEvent': | ||
| return new BoolExpressionConverter(); | ||
| case 'disabled': | ||
| return new BoolExpressionConverter(); | ||
| default: |
There was a problem hiding this comment.
Note: This is just to collapse the case statements since they're duplicates.
mdrichardson
changed the title
Pull Request Test Coverage Report for Build 850767067
💛 - Coveralls |
| /** | ||
| * The property path to store whether the event was handled or not. | ||
| */ | ||
| public handledProperty: StringExpression; |
There was a problem hiding this comment.
Should this be public handledProperty?: StringExpression;?
There was a problem hiding this comment.
I'm not sure, but that comment did make me realize I forgot to default it to turn.eventHandled.
If you're questioning the ? because of:
export interface EmitEventConfiguration extends DialogConfiguration {
[...]
handledProperty?: StringProperty;
}...I'm not really sure. The interface and class property optionality is kind of inconsistent across the JS Adaptive library, but it seems like it's mostly:
- Optional in the interface
- Required in the class property
There was a problem hiding this comment.
It's likely only required in the class property due to not using strict mode. IMO the class and the interface should match. In this case, if there is a default value, the property should not be marked as optional.
There was a problem hiding this comment.
So, make it required in the ...Configuration interface, as well, then? None of the class properties are marked optional; should I also make the rest of them required in the interface? Again, normally I would, but there isn't really a pattern in the Adaptive library for this. Actually, I think just about all of the ...Configuration interfaces across adaptive have all-optional keys.
There was a problem hiding this comment.
Let's sync up on a call this week to discuss, I think we can make some more general improvements. In the meantime, I won't hold this PR up.
Fixes #3594
Description
Added a new "Handled Property" property which lets you specify where to return a flag indicating if the event was handled.