Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

Templated RadioButtons and RadioButtonGroups #11628

Merged
merged 32 commits into from
Sep 18, 2020
Merged

Templated RadioButtons and RadioButtonGroups #11628

merged 32 commits into from
Sep 18, 2020

Conversation

hartez
Copy link
Contributor

@hartez hartez commented Jul 31, 2020

Description of Change

Control Templates

[Update: we've moved back to the ToString() behavior instead of exceptions; also, UWP now natively supports arbitrary Forms Views for Content.]

These changes make RadioButton a TemplatedView. This allows for the use of ControlTemplate (to make RadioButton look and behave the same on all platforms), or for the use of a native Renderer (to utilize the native RadioButton control available on platforms which support it).

This also allows for the RadioButton to fall back to the default ControlTemplate on iOS, which does not have a RadioButton control.

The RadioButton no longer has a Text property. Instead, it has a Content property which can be set to string or to arbitrary content. If the Content property is set to string, that string is used as the displayed text on any native renderers. Any platform using a ControlTemplate will display the string content as a Label.

If the Content property is set to View, the native renderers which do not support arbitrary content will simply display the result of the ToString() method. ControlTemplates will display the View in the ContentPresenter section of the template. Platforms which support arbitrary content natively (i.e., UWP) will convert the View to a native renderer and display that.

So this works on all platforms/renderers:
<RadioButton Content="Option A" />

If we set a ControlTemplate explicitly, we can use arbitrary Views as Content on all platforms:

<RadioButton ControlTemplate="{x:Static RadioButton.DefaultTemplate}">
    <RadioButton.Content>
        <Image Source="coffee.png"/>
    </RadioButton.Content>
</RadioButton>

Setting arbitrary Views as Content without using a ControlTemplate will simply display the string representation. So on Android, the following markup:

<RadioButton>
    <RadioButton.Content>
        <Image Source="coffee.png"/>
    </RadioButton.Content>
</RadioButton>

Will simply display the string "[Xamarin.Forms.Image]". On UWP, the Forms Image will be converted to a UWP Image and displayed. Because iOS is always using a template for RadioButton - there is no native renderer on that platform - an Image will be displayed.

RadioButtonGroups

These changes also introduce RadioButtonGroup attached properties, which allow any Layout to be turned into a radio button group:

<StackLayout RadioButtonGroup.GroupName="foo">
    <RadioButton Content="Option A" />
    <RadioButton Content="Option B" />
    <RadioButton Content="Option C" />
</StackLayout>

All of the RadioButtons in the StackLayout above will be given the GroupName of "foo" automatically, and will be mutually exclusive.

<StackLayout RadioButtonGroup.GroupName="{Binding GroupName}" 
                         RadioButtonGroup.SelectedValue="{Binding Selection}">
    <RadioButton Content="Option A" Value="yes" />
    <RadioButton Content="Option B" Value="no" />
    <RadioButton Content="Option C" Value="maybe" />
</StackLayout>

All of the RadioButtons in the StackLayout above will be give a GroupName based on the bound value of GroupName in the BindingContext. The Value property of whichever RadioButton is selected will be bound to the Selection property in the BindingContext.

Issues Resolved

None

API Changes

RadioButton is no longer a subclass of Button; rather, it inherits from TemplatedView

  • Removed the Text property
  • Added the Content property
  • Added the Value property

Added class:

public static class RadioButtonGroup
public static readonly BindableProperty GroupNameProperty
public static readonly BindableProperty SelectedValueProperty

Platforms Affected

  • Core/XAML (all platforms)
  • iOS
  • Android
  • UWP
  • WPF
  • Tizen
  • macOS

Behavioral/Visual Changes

None

Before/After Screenshots

Not applicable

Testing Procedure

Control Gallery -> RadioButton Galleries

Also a bunch of unit tests

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)

@andreinitescu
Copy link
Contributor

If the Content property is set to View, the native renderers will throw an exception

It's important not to throw an exception in my opinion, the native renderers can just use Content?.ToString() instead.

@andreinitescu
Copy link
Contributor

andreinitescu commented Aug 1, 2020

Quite interesting you decided to go with such a breaking change, I remember piles of discussion of "oh, no, we can't do this, it would be a breaking change". Happy to see you're moving forward!

@hartez
Copy link
Contributor Author

hartez commented Aug 1, 2020

A case we haven't considered here is what happens if a ControlTemplate is set after the native renderer has already been selected (and is possibly already on screen)? The easy option would be to simply ignore it - is that reasonable?
@PureWeen Any thoughts on this? What happens when a Visual changes at runtime?

@hartez
Copy link
Contributor Author

hartez commented Aug 1, 2020

It's important not to throw an exception in my opinion, the native renderers can just use Content?.ToString() instead.

That's what the first draft did. The reason for the change was to make it as obvious as possible to the user that what they are trying to do won't work. And to give them an explicit message explaining why (instead of just seeing weird behavior like a RadioButton with a label text of "[Xamarin.Forms.Image]").

The "property order" problem isn't as big here as it would be if we were doing it in Core - we don't have to worry about XAML order or anything like that, because these exceptions don't happen until we've reached the native renderers. By definition, we couldn't be in this stage if ControlTemplate were anything but null.

Can we come up with a concrete example of a situation where this behavior would cause confusion or catastrophe? (That might depend on the outcome of my previous comment),

@bmacombe
Copy link
Contributor

bmacombe commented Aug 1, 2020

If the Content property is set to View, the native renderers will throw an exception. ControlTemplates will display the View in the ContentPresenter section of the template.

When I first read that I didn't catch the "View" part. So would it only throw if you put a view as the content and not other random object or binding?

If it's only if you assign a View subclass, then I think the exception is good. But let's say I bind the content to some random object, I would expect it to just ToString it.

What I wouldn't want is a scenario like if you use a ListView but forget to put a ViewCell in the DataTemplate and it just doesn't show anything.

@andreinitescu
Copy link
Contributor

Like I said on Twitter, in my opinion having a specific mandatory order for setting properties otherwise it throws an exception is not a good design. I don’t remember any API in .NET which works like this.
I don’t have an example right now when it won’t actually work. Even if there is none, the fact I have to set properties in a specific order doesn’t sound good to me.
I realized you want to throw an exception so the developer can see it, but in this particular scenario I don’t think it’s a good decision.
At most you could write a warning message to the device log.

@andreinitescu
Copy link
Contributor

Beside a warning in device log, having a clear concise comment on the Content property is effective too

@samhouts samhouts changed the base branch from 4.8.0 to 5.0.0 August 4, 2020 21:45
@samhouts samhouts added this to the 5.0.0 milestone Aug 4, 2020
@samhouts samhouts removed this from In Review in vCurrent (4.8.0) Aug 4, 2020
@samhouts samhouts added this to In Review in vNext+1 (5.0.0) Aug 11, 2020
@PureWeen
Copy link
Contributor

Overall works great.

Should this still render text into the ContentPresenter?

        <RadioButton>
            <RadioButton.ControlTemplate>
                <ControlTemplate>
                    <StackLayout Orientation="Horizontal">
                        <ContentPresenter></ContentPresenter>
                    </StackLayout>
                </ControlTemplate>
            </RadioButton.ControlTemplate>
            <RadioButton.Content>
                Rabbits
            </RadioButton.Content>
        </RadioButton>

Any thoughts on this? What happens when a Visual changes at runtime?

Nothing happens when you change visual at runtime. Visual is such a drastic change with how a field is displayed that it's not a scenario people have ran into.

Here's the Issue about it
#5482
No thumbs up on it yet :-)

I would default the direction of just doing nothing for now and seeing how people use it. It's weird to switch from the native renderer to the xplat renderer because the radiobutton itself renderers so different. If specifying the ControlTemplate created a completely symmetrical RadioButton then I think it'd make a bit more sense but I feel like you're either going to do one or the other.

Here was the PR I had started for visual which would cause elements to recreate
b3d53cb

It was just a really quick spike but I feel like not worrying about this case for now makes the most sense

some thoughts on exceptions

If we keep the current behavior I'm a fan of the exceptions.

I type the following sentence a lot "did you look at your application output for warnings about bindings?". If I specify an Image or a Label for my content and nothing displays on android but it does display on iOS my first thought will be that there's a bug on Android

Going the exception route

  • Should Android throw an exception when you specify complex content so that the user is aware of why the content won't display?
  • If we keep the exceptions should they refer to the platform and not the renderer?
    $"WPF only supports string values for the {nameof(RadioButton)} {RadioButton.ContentProperty.PropertyName} property."
    vs
    $"{nameof(RadioButtonRenderer)} only supports string values for the {nameof(RadioButton)} {RadioButton.ContentProperty.PropertyName} property."
  • Can we come up with a concrete example of a situation where this behavior would cause confusion or catastrophe? I can't off hand but this might just be a failure of imagination because, like you said, these only throw once they hit the rendering stage of life. The main case I can think of here is with CollectionView if you have something like a DataTemplateSelector where some are native and some aren't, but again I feel like that's not going to come up.

The just make it work option opposed to exceptions

For the indicators @rmarinho made is so if you specify a template it just uses the xplat indicators and if you don't then it uses the native ones. Would it make sense to follow a similar strategy here?

So if I specify

<RadioButton>
            <RadioButton.Content>
                <Image Source="coffee.png"/>
            </RadioButton.Content>
        </RadioButton>

It should just opt in to use the DefaultTemplate? I refer back to this statement If I specify an Image for my content and nothing displays on android but it does display on iOS then my first thought will be that there's a bug on Android.

UncheckedButton and CheckedIndicator

Do those names come from something?

I don't feel like UncheckedButton is accurate because for me that implies it won't be visible when you've selected the radiobutton but the UncheckedButton is visible.

Maybe "Button" and ("CheckedIndicator" or "Checkmark" or "RadioButton")

@PureWeen
Copy link
Contributor

I know we had chatted a bit where visual fits into all of this which opens a few other options. @jsuarezruiz, @rmarinho , and I have talked about this a bit with other implementations. For example when @rmarinho first did the indicators he had added a "FORMS" Visual which indicated to the control that it should use xplat instead of platform. We eventually got rid of that for indicators because it didn't really offer a different experience setting the visual to "forms" vs "default"

With RadioButton that's a bit different, especially once material and fluent enter the mix. There were a few ideas we've been floating around visual
#5005

So for RadioButton it would look something like

<RadioButton>
     <RadioButton.VisualTemplate>
         <Forms Content="Text" UncheckedButton="" ControlTemplate=""   />
     </RadioButton.VisualTemplate>
</RadioButton>
<RadioButton>
     <RadioButton.VisualTemplate>
         <Native Text="Text"  />
     </RadioButton.VisualTemplate>
</RadioButton>

We would still let people specify "Content" on the RadioButton itself but then we could add discoverability of customization this way.

@mattleibow
Copy link
Contributor

Hey folks, I am having a look at the basic TemplatedView and came across an "issue" with the app not automatically importing styles. I created this issue (#11798) which has a bit more info. I think something like this should be done by the framework.

@rmarinho
Copy link
Member

Failing tests were not related.

@rmarinho rmarinho merged commit 2a97a6c into 5.0.0 Sep 18, 2020
vNext+1 (5.0.0) automation moved this from In Review to Done Sep 18, 2020
@rmarinho rmarinho deleted the radiobuttons branch September 18, 2020 16:38
@samhouts samhouts added the approved Has two approvals, no pending reviews, and no changes requested label Sep 18, 2020
@samhouts samhouts added this to Done in Sprint 176 Sep 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

8 participants