Skip to content

Conversation

@danielchalmers
Copy link
Member

Description

You should never have a lone toggle item and it breaks the styles so why design the code around that?

How Has This Been Tested?

Type of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation (fix or improvement to the website or code docs)
crit: Microsoft.AspNetCore.Components.WebAssembly.Rendering.WebAssemblyRenderer[100]
      Unhandled exception rendering component: MudToggleItem must be used within a MudToggleGroup.
System.InvalidOperationException: MudToggleItem must be used within a MudToggleGroup.
   at MudBlazor.MudToggleItem`1[[System.String, System.Private.CoreLib, Version=9.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].get_Parent() in C:\Users\danch\source\repos\MudBlazor\src\MudBlazor\Components\Toggle\MudToggleItem.razor.cs:line 46
   at MudBlazor.MudToggleItem`1[[System.String, System.Private.CoreLib, Version=9.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].OnInitialized() in C:\Users\danch\source\repos\MudBlazor\src\MudBlazor\Components\Toggle\MudToggleItem.razor.cs:line 135
   at Microsoft.AspNetCore.Components.ComponentBase.RunInitAndSetParametersAsync()
   at MudBlazor.State.ParameterContainer.SetParametersAsync(Func`2 baseSetParametersAsync, ParameterView parameters) in C:\Users\danch\source\repos\MudBlazor\src\MudBlazor\State\ParameterContainer.cs:line 84

Checklist

  • The PR is submitted to the correct branch (dev).
  • My code follows the code style of this project.
  • I've added relevant tests.

This comment was marked as outdated.

@danielchalmers danielchalmers requested a review from Copilot June 14, 2025 20:34
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enforces that MudToggleItem must always be used within a MudToggleGroup by validating the cascading parent and removing null‐conditional logic throughout the component.

  • Adds a private non-nullable Parent property that throws an exception if no group is found.
  • Renames the public cascading parameter to NullableParent and updates registration/unregistration to use it.
  • Removes ?. checks in class builders and lifecycle methods in favor of the validated Parent property.
Comments suppressed due to low confidence (4)

src/MudBlazor/Components/Toggle/MudToggleItem.razor.cs:41

  • [nitpick] The public cascading parameter property NullableParent name is ambiguous; consider renaming it to something like CascadingParent or ParentCascading to better convey its purpose.
public MudToggleGroup<T>? NullableParent { get; set; }

src/MudBlazor/Components/Toggle/MudToggleItem.razor.cs:134

  • Consider adding a unit test to verify that a MudToggleItem without a parent throws the expected InvalidOperationException during initialization.
Parent.Register(this);

src/MudBlazor/Components/Toggle/MudToggleItem.razor.cs:41

  • [nitpick] Add an XML <summary> comment for the NullableParent cascading parameter property to explain its purpose and why it differs from the private Parent property.
public MudToggleGroup<T>? NullableParent { get; set; }

src/MudBlazor/Components/Toggle/MudToggleItem.razor.cs:27

  • Previously there was a fallback to Size.Medium when Parent.Size was null; the new code always uses Parent.Size. Ensure Parent.Size is always initialized or reintroduce a fallback to avoid unexpected default values.
.AddClass($"mud-toggle-item-size-{Parent.Size.ToDescriptionString()}")

@ScarletKuro
Copy link
Member

Is this a change for v9? Since it's breaking change.
You renamed

public MudToggleGroup<T>? Parent { get; set; }

to

public MudToggleGroup<T>? NullableParent { get; set; }

Also I'm not really fan of that Nullable naming in public API.
Why don't you just make a private\internal NonNullParent and use it in the code instead of touching the public API?

@danielchalmers
Copy link
Member Author

Is this a change for v9? Since it's breaking change. You renamed

public MudToggleGroup<T>? Parent { get; set; }

to

public MudToggleGroup<T>? NullableParent { get; set; }

Also I'm not really fan of that Nullable naming in public API. Why don't you just make a private\internal NonNullParent and use it in the code instead of touching the public API?

just made NullableParent private since the goal is to encapsulate the whole component. Are people setting Parent manually though? I don't see it breaking anyone's code because the component is useless unless it's directly inside the actual parent

@danielchalmers danielchalmers changed the title MudToggleItem: Enforce parent hierarchy MudToggleItem: Enforce direct parent hierarchy Jun 14, 2025
@codecov
Copy link

codecov bot commented Jun 14, 2025

Codecov Report

Attention: Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.

Project coverage is 91.15%. Comparing base (5df9ab1) to head (ab0854f).
Report is 5 commits behind head on dev.

Files with missing lines Patch % Lines
...MudBlazor/Components/Toggle/MudToggleItem.razor.cs 90.90% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##              dev   #11498   +/-   ##
=======================================
  Coverage   91.14%   91.15%           
=======================================
  Files         466      466           
  Lines       14497    14499    +2     
  Branches     2816     2815    -1     
=======================================
+ Hits        13214    13217    +3     
  Misses        644      644           
+ Partials      639      638    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ScarletKuro
Copy link
Member

ScarletKuro commented Jun 14, 2025

just made NullableParent private since the goal is to encapsulate the whole component. Are people setting Parent manually though? I don't see it breaking anyone's code because the component is useless unless it's directly inside the actual parent

It's not about setting though. Someone might have accessed the Parent in their custom code, and now you are removing it = breaking change. We do not break userspace (c) Linus.

@danielchalmers
Copy link
Member Author

just made NullableParent private since the goal is to encapsulate the whole component. Are people setting Parent manually though? I don't see it breaking anyone's code because the component is useless unless it's directly inside the actual parent

It's not about setting though. Someone might have accessed the Parent in their custom code, and now you are removing it = breaking change. We do not break userspace (c) Linus.

Mmm im skeptical but i understand what youre going for so swapped the names around and now Parent is the same as before

@danielchalmers danielchalmers requested a review from mckaragoz June 14, 2025 20:53
@sonarqubecloud
Copy link

@ScarletKuro
Copy link
Member

Mmm im skeptical

Sample code
<MudToggleGroup T="string" SelectionMode="SelectionMode.SingleSelection" @bind-Value="_value1" Color="Color.Primary" CheckMark FixedContent>
  <MudToggleItem @ref="_toggleItem" Value="@("Yes")" Text="Yes" />
  <MudToggleItem Value="@("No")" Text="No" />
  <MudToggleItem Value="@("Don't know")" Text="Don't know" />
</MudToggleGroup>
<MudButton Variant="Variant.Filled" Color="Color.Primary" OnClick="ButtonOnClick">Invoke Me</MudButton>

@code {
  private string _value1 = "Yes";
  private MudToggleItem<string> _toggleItem;

  private void ButtonOnClick()
  {
      var value = _toggleItem.Parent.Value; //Ups if you remove it.
      Console.WriteLine(value);
  }
}

It doesn’t matter and it doesn't matter how far-fetched or contrived this example may seem, or how low the risk is. We don't know how others write their code, so we can't just add an API and then remove it. If we want to do that, it should only happen in the next major version. We’ve been through this before and were criticized for it when we pulled something similar in non major version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants