Skip to content
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

RadioButtons: Custom ItemTemplate is not automatically prefaced by a RadioButton if it doesn't specify one itself as its root element #3159

Closed
Felix-Dev opened this issue Aug 19, 2020 · 14 comments · Fixed by #3194
Labels
area-RadioButtons team-Controls Issue for the Controls team

Comments

@Felix-Dev
Copy link
Contributor

Felix-Dev commented Aug 19, 2020

Describe the bug
According to #333 (comment) by @kikisaints, a custom ItemTemplate for the RadioButtons control will be prefaced by a RadioButton icon:

Meaning you can customize your data template in any way you like, and get that exact definition represented in the RadioButton group, but prefaced with a RadioButton icon.

Thus, if we have the following XAML:

<muxc:RadioButtons Header="Test Group"
                   ItemsSource="{x:Bind Options}">
    <muxc:RadioButtons.ItemTemplate>
        <DataTemplate x:DataType="local:OptionDataModel">
            <StackPanel Orientation="Horizontal">
                <TextBlock Text="{x:Bind Label}" Margin="5"/>
                <HyperlinkButton Content="My Link" Margin="5"/>
                <Rectangle Fill="Orange" Width="10" Height="10" Margin="5"/>
            </StackPanel>
        </DataTemplate>
    </muxc:RadioButtons.ItemTemplate>
</muxc:RadioButtons>

and code-behind like this:

public class OptionDataModel
{
    public string Label { get; set; }
}

public IList<OptionDataModel> Options { get; set; }

public MainPage()
{
    this.InitializeComponent();

    Options = new List<OptionDataModel>
    {
        new OptionDataModel() { Label = "Item 1" },
        new OptionDataModel() { Label = "Item 2" },
        new OptionDataModel() { Label = "Item 3" },
        new OptionDataModel() { Label = "Item 4" },
        new OptionDataModel() { Label = "Item 5" },
    };
}

The expected UI is this:
image

But the actual UI we get is this:
image

As you can see, there are no RadioButton icons, plus our items are no longer represented as RadioButtons internally, thus we cannot select our options in a mutually exclusive way (actually, we cannot select them at all).

Version Info

NuGet package version:
Newest WinUI 2.5 preview.

Windows 10 version Saw the problem?
Insider Build (xxxxx)
May 2020 Update (19041) Yes
November 2019 Update (18363)
May 2019 Update (18362)
October 2018 Update (17763)
April 2018 Update (17134)
Fall Creators Update (16299)
Creators Update (15063)
Device form factor Saw the problem?
Desktop Yes
Xbox
Surface Hub
IoT

Additional context
I would like to fix this.

@msft-github-bot msft-github-bot added the needs-triage Issue needs to be triaged by the area owners label Aug 19, 2020
@robloo
Copy link
Contributor

robloo commented Aug 19, 2020

It seems like the current functionality might be by design. How do you change the look of the radio button itself otherwise?

@Felix-Dev
Copy link
Contributor Author

Felix-Dev commented Aug 19, 2020

@robloo The issue is that the current impementation makes the RadioButtons control more or less useless when a custom ItemTemplate is specified. You cannot select items mutually exclusive any longer, let alone select them at all. This is the single main purpose of this control though!

If you want to style the RadioButton "icon" itself, my guess was you could do it like this:

<muxc:RadioButtons>
    <muxc:RadioButtons.Resources>
        <Style x:Key="DefaultRadioButtonStyle" TargetType="RadioButton">
            <!---->
        </Style>
    </muxc:RadioButtons.Resources>
    <Button Content="OptionButton1"/>
    <Button Content="OptionButton2"/>
</muxc:RadioButtons>

so you overwrite the default RadioButton style in use. Unfortunately, that does not appear to work.

Also, I think adding a RadioButton icon by default is the correct step here, as we are in a RadioButtons control, so RadioButtons are expected here. Mutually-exclusive selection UIs like these are well-known to be represented by RadioButtons and I believe developers expect RadioButtons to show up here. So when they specify a custom ItemTemplate (for example when they use Data-binding), they shouldn't also have to specify a RadioButton as well. This should come with the control automatically. (I imagine that was the design thought when @kikisaints gave that example in the linked comment above.)

I agree with you though that the RadioButton itself should be stylable. I would expect that to work just like I would normally style a RadioButton (by specifying a new RadioButton style), just that here we overwrite the existing default RadioButton style.

@robloo
Copy link
Contributor

robloo commented Aug 19, 2020

The issue is that the current impementation makes the RadioButtons control more or less useless when a custom ItemTemplate is specified.

I definitely see there is an issue here, but we don't want to make it useless for those that need to change the radio button style too :)

Also, I think adding a RadioButton icon by default is the correct step here, as we are in a RadioButtons control, so RadioButtons are expected here.

I couldn't agree more!

I agree with you though that the RadioButton itself should be stylable. I would expect that to work just like I would normally style a RadioButton (by specifying a new RadioButton style), just that here we overwrite the existing default RadioButton style.

I agree with your thinking overall, we just have to figure out how to support styling the radio button itself too separate from the item content if needed. Changing the DefaultRadioButtonStyle across the board should be supported -- but it should be possible to do this on a per-control basis as well. (Actually, UWP needs implicit styles and to also define keys for all system default styles... separate issue). Adding a new property for the style of the RadioButton itself might work.

We also have to consider right-to-left text layouts where the RadioButton should probably be on the right-hand side.

@ranjeshj
Copy link
Contributor

Would the below pattern work ? That way you can set the properties on the RadioButton directly and don't need a Style.

<muxc:RadioButtons Header="Test Group"
                   ItemsSource="{x:Bind Options}">
    <muxc:RadioButtons.ItemTemplate>
        <DataTemplate x:DataType="local:OptionDataModel">
         **<RadioButton>**
            <StackPanel Orientation="Horizontal">
                <TextBlock Text="{x:Bind Label}" Margin="5"/>
                <HyperlinkButton Content="My Link" Margin="5"/>
                <Rectangle Fill="Orange" Width="10" Height="10" Margin="5"/>
            </StackPanel>
          **</RadioButton>**
        </DataTemplate>
    </muxc:RadioButtons.ItemTemplate>
</muxc:RadioButtons>

@Felix-Dev
Copy link
Contributor Author

Felix-Dev commented Aug 19, 2020

@ranjeshj That works as expected (only adjusted some Margins for better design):

image

The question is: Is this just a workaround? Or Is this how the ItemTemplate has to be constructed (RadioButton at the root)? In other words, do developers have to explictily specify a root RadioButton in the ItemTemplate? If so, how should we handle an ItemTemplate which lack a RadioButton as its root element?

@ranjeshj
Copy link
Contributor

I see. If the RadioButton containers are automatically created by RadioButtons control, then there is no easy way to style that without exposing a Style/StyleSelector.

@robloo
Copy link
Contributor

robloo commented Aug 19, 2020

A DataTemplate that requires a RadioButton as the root element actually makes sense to me in this case. It is a RadioButtons control after all. Technically speaking, are there actually any limitations with @ranjeshj solution? It seems to me it works in all cases. If you aren't using a RadioButton in the style then another repeater control should be used. We should just add a note in the documentation for this.

@Felix-Dev
Copy link
Contributor Author

Felix-Dev commented Aug 19, 2020

@robloo @ranjeshj How about this: If we don't specify a RadioButton as the root of our ItemTemplate, then WinUI will wrap that ItemTemplate in a RadioButton. No easy RadioButton customization available.

However, if more styling flexibility for the RadioButton itself is required, I can specify a RadioButton as the root of the ItemTemplate and set all kinds of properties on it. In that case, WinUI won't wrap a RadioButton around the ItemTemplate as it already is a RadioButton.

(The TreeView control for example does something like this with its ItemTemplate - if no TreeViewItem is specified, the ItemTemplate is wrapped into a TreeViewItem.)

@Felix-Dev
Copy link
Contributor Author

Felix-Dev commented Aug 19, 2020

@robloo Do you think we should also add an ItemTemplateSelector API to the RadioButtons control? That API is missing so far and would give even more fine granular styling possibilities. Plus, it would help bring the same level of customization options to the RadioButtons control as known from other such Item controls.

@robloo
Copy link
Contributor

robloo commented Aug 19, 2020

@robloo Do you think we should also add an ItemTemplateSelector API to the RadioButtons control?

It wouldn't hurt. However, I'm not sure it's really justified. The RadioButons is not a general-purpose control. I can't really envision scenarios where we would need to change the template... Except maybe for when an item is checked.

@Felix-Dev Felix-Dev changed the title RadioButtons: Custom ItemTemplate is not prefaced by a RadioButton RadioButtons: Custom ItemTemplate is not automatically prefaced by a RadioButton if it doesn't specify one itself as its root element Aug 19, 2020
@Felix-Dev
Copy link
Contributor Author

Felix-Dev commented Aug 19, 2020

@robloo Yeah, I do not recall ItemTemplateSelector being an API request until now. Thta might change but it seems like we don't have to invest resources into that for now.

And yes, we should consider adding documentation how to customize the actual RadioButtons for the RadioButtons control (for example by specifying an ItemTemplate with a RadioButton as its root element). It might be obvious for some developers but perhaps not for others.

@StephenLPeters StephenLPeters added area-RadioButtons team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Aug 19, 2020
@Felix-Dev
Copy link
Contributor Author

Pinging @ranjeshj here in cased initial ping got missed (ping created during recent WinUI call, perhaps was bad timing). Your thoughts on this?

@ranjeshj
Copy link
Contributor

Sorry missed the ping.. I'm also thinking there likely isn't much to do here.

If the root of the template is not a RadioButton, we could create one automatically for you which will fix this.

The issue is that if RadioButtons automatically created a RadioButton for you, there is no explicit property to style that RadioButton. You could add a style to RadioButtons.ResourceDictionary. Alternatively you could provide your own RadioButton in the template and update its properties directly. This part probably just needs better documentation.

@Felix-Dev
Copy link
Contributor Author

Felix-Dev commented Aug 21, 2020

@ranjeshj
Yeah, I like the idea that we will implicitly create a RadioButton to wrap around the ItemTemplate (if it isn't already a RadioButton), yet if you need to customize the RadioButton appearance, then you can just create an ItemTemplate with an explicit RadioButton as its root.

So, to summarize: What is to do here is to

  • provide the implicit wrapping functionality for the ItemTemplate if not already a RadioButton and
  • improve the documentation to show how the RadioButton can be customized.

I would like to work on both of these items.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-RadioButtons team-Controls Issue for the Controls team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants