-
Notifications
You must be signed in to change notification settings - Fork 540
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
[Shared/UWP] Add IsEnabled property to actions #5734
Conversation
f41cf54
to
99788c1
Compare
@@ -617,9 +625,19 @@ namespace AdaptiveNamespace::ActionHelpers | |||
ComPtr<IAdaptiveHostConfig> hostConfig; | |||
THROW_IF_FAILED(renderContext->get_HostConfig(&hostConfig)); | |||
|
|||
if (ActionHelpers::WarnForInlineShowCard(renderContext, action, L"Inline ShowCard not supported for SelectAction")) | |||
// We don't wrap the element if it's a show card or if the action's not enabled; | |||
bool dontWrap = ActionHelpers::WarnForInlineShowCard(renderContext, action, L"Inline ShowCard not supported for SelectAction"); |
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.
could we make this shouldWrap
-- we should avoid negatives in bools :D
@@ -90,6 +90,15 @@ namespace AdaptiveNamespace | |||
return AdaptiveActionElementBase::put_Tooltip(tooltip); | |||
} | |||
|
|||
IFACEMETHODIMP put_IsEnabled(boolean isEnabled) override |
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.
definitely not something to do in this PR, but I think if we refactor our classes using cppwinrt, we should be able to derive classes in a way that makes it so we can get out of the business of manually calling the base (LinkButton
does something similar to this).
guessing it might require breaking API surface changes, so maybe part of our next major rev? just a thought :)
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 agree, I'd love to get more cppwinrt in here. I feel like it shouldn't require breaking changes? But I don't know. Definitely something to work towards at some point.
* [Shared] Add IsEnabled to the shared model * [UWP] Add IsEnabled to the object model * [UWP] Renderer Support * PR feedback
* [Shared] Add IsEnabled to the shared model * [UWP] Add IsEnabled to the object model * [UWP] Renderer Support * PR feedback
Related Issue
Fixes #5723
Fixes #5721
Description
Add isEnabled property to actions in the shared model and UWP and updated the UWP renderer code to respect it
Sample Card
New sample card added samples\v1.5\Elements\Action.IsEnabled.json
How Verified
Microsoft Reviewers: Open in CodeFlow