search

CommunityToolkit / Windows

Collection of controls for WinUI 2, WinUI 3, and Uno Platform developers. Simplifies and demonstrates common developer tasks building experiences for Windows with .NET.
https://aka.ms/windowstoolkitdocs
Other
596 stars 74 forks source link

SettingsExpander in SettingsExpander crashes the application #560

Open naumenkoff opened 1 week ago

naumenkoff commented 1 week ago

Describe the bug

If you define a SettingsExpander as an item within another SettingsExpander, the application crashes. 

<controls:SettingsExpander Header="Parent">
    <controls:SettingsExpander.Items>
        <controls:SettingsExpander Header="Child" />
    </controls:SettingsExpander.Items>
</controls:SettingsExpander>

Steps to reproduce

1. Add a `SettingsExpander` to the layout.  
2. Add another `SettingsExpander` as a nested item inside the first `SettingsExpander`.  
3. Run the application.  
4. Observe that the application crashes.

Expected behavior

The application should not crash.

Screenshots

No response

Code Platform

Windows Build Number

Other Windows Build number

Windows 11 24H2 (Build 26100)

App minimum and target SDK version

Other SDK version

No response

Visual Studio Version

Preview

Visual Studio Build Number

17.12.0 Preview 5.0

Device form factor

Desktop

Additional context

CommunityToolkit.WinUI.Controls.SettingsControls: 8.1.240916

Help us help you

No, I'm unable to contribute a solution.

michael-hawker commented 5 days ago

@niels9001 this isn't something I think we ever considered with the Settings pattern, eh?

I would guess it's related to #302 as SettingsExpander expects SettingsCards as children, and SettingsExpander itself (while looking and using a Card, isn't one itself).

Is this a special case we should allow? Or is this an odd design pattern we want to discourage?

@naumenkoff can you share the scenario for what you were trying to achieve?

naumenkoff commented 5 days ago

@niels9001 this isn't something I think we ever considered with the Settings pattern, eh?

I would guess it's related to #302 as SettingsExpander expects SettingsCards as children, and SettingsExpander itself (while looking and using a Card, isn't one itself).

Is this a special case we should allow? Or is this an odd design pattern we want to discourage?

@naumenkoff can you share the scenario for what you were trying to achieve?

These are just my observations 🙃 I don't think that this is a bad thing, because everything has its own limitations and it would be good to mention it somewhere (Gallery?) (I didn't find such information)

niels9001 commented 5 days ago

Yeah, this isn't a pattern I have seen before across Windows. I can imagine it might come with some accesibility issues as well when nesting different expanders?

@naumenkoff do you have a specific UI in mind with the code you provided?

@michael-hawker @Arlodotexe is there a straightforward way to throw a warning or something else when not using a SettingsCard?

naumenkoff commented 5 days ago

Yeah, this isn't a pattern I have seen before across Windows. I can imagine it might come with some accesibility issues as well when nesting different expanders?

@naumenkoff do you have a specific UI in mind with the code you provided?

@michael-hawker @Arlodotexe is there a straightforward way to throw a warning or something else when not using a SettingsCard?

No, sorry, I don’t have a UI in mind with this approach because I don’t like deep nesting. I actually stumbled upon this completely by accident during a refactor when I wanted to group several SettingsCards (the first being a ToggleSwitch and the second a TextBox) under a single SettingsExpander and accidentally replaced them both with a SettingsExpander.

Image