[Android] Always apply configured Actions.IconSize #5704
Conversation
|
I think the choice of 30px as a default is what is problematic here. The typical size of a button icon is 16px. I think you should change the default to 16px, which is what JS uses. |
@dclaux |
|
My recollection was that it was intentional to use text height rather than icon size when the icon is left of the title, and in general I think that's a good approach. What would the above title button look like if we size the default down to 16? I suspect it would look a little too small in that case. For what it's worth the default of 30 is consistent with the documentation (ActionsConfig). That said, it is weird to have an iconSize property but not always respect it (especially given that that behavior is not documented). I like @golddove's idea of having it be optional and having the renderer default to text height for left of title and the documented default (30) for above title. That seems like the best of both worlds in terms of letting us do the reasonable default but also respecting any explicit value from the host. In any case, if we stick with the approach in this PR we'll need to check the other platforms as well as I suspect everyone except JavaScript is doing it the way Android was. We should also make sure to update the docs with whatever we land on. |
16 looks fine on Android:
|
|
I only suggested using 16 in place of 30 where 30 is currently used as a default, not to change other logic such as using title height. |
|
Documenting outcome from standup: match current JS behavior (i.e. always respect iconSize, and default to 16 when unset) |
| @@ -292,7 +292,7 @@ namespace AdaptiveSharedNamespace | |||
| unsigned int maxActions = 5; | |||
| Spacing spacing = Spacing::Default; | |||
| IconPlacement iconPlacement = IconPlacement::AboveTitle; | |||
| unsigned int iconSize = 30; | |||
| unsigned int iconSize = 16; | |||
There was a problem hiding this comment.
@golddove
How would we know if the iconSize is not set? Shouldn't we change it to optional to tell if it has a value?
There was a problem hiding this comment.
The plan of record was to always respect iconSize.
ghost
added
the
Logged UWP bug, and added comment to existing iOS bug, |
* [Android] Always apply configured Actions.IconSize * Default to 16 * Updated specs Co-authored-by: Adaptive Cards Specs CI <donotreply@microsoft.com> Co-authored-by: RebeccaAnne <rebecch@microsoft.com>
* [Android] Always apply configured Actions.IconSize * Default to 16 * Updated specs Co-authored-by: Adaptive Cards Specs CI <donotreply@microsoft.com> Co-authored-by: RebeccaAnne <rebecch@microsoft.com>
* [Android] Always apply configured Actions.IconSize * Default to 16 * Updated specs Co-authored-by: Adaptive Cards Specs CI <donotreply@microsoft.com> Co-authored-by: RebeccaAnne <rebecch@microsoft.com>
Related Issue
Fixes #5567
Description
Fixes Android renderer to match JS behavior of applying HostConfig.ActionsConfig.IconSize always, not just when placement is above title. Previously, we were setting image height to match text height when placement is left of title.
However, this change results in a poor default (see screenshot below), because the default is arbitrarily set to 30 in the shared model. So, any host currently using
iconPlacement: LeftOfTitlewill need to newly configure theiriconSizeto restore the previous size.If this is not a default we want, we could remove the arbitrary default in the object model (and make it optional), to allow the renderer to use a good default (i.e. icon height matches text height).
Sample Card
v1.2\Elements\Action.IconUrl.TwoActionsHow Verified
(using default host config values)
Before PR:
After PR:
Microsoft Reviewers: Open in CodeFlow