MudList: Add Nullable and use ParameterState

[ScarletKuro]

Description

Contributes to #6535 and #8258

How Has This Been Tested?

Current unit test + visual doc tests.

Types 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)

Checklist:

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

[ScarletKuro]

Would be nice if someone can do additional testing if they have some other scenario usage.

[ScarletKuro]

Unfortunately I don't know how to avoid this moment with ParameterState framework

MudBlazor/src/MudBlazor/Components/List/MudList.razor.cs

Lines 186 to 193 in f9eebe1

	// We cannot use the _selectedValueState.Value here instead of _selectedValue
	// The problem arises when the SelectedValue is preselected
	// Then OnInitialized will set the _selectedValueState.Value = SelectedValue
	// And this will early exit, while we need it to be null for this complicated logic to work like it was before
	if (Equals(_selectedValue, value))
	{
	return;
	}

I believe it's just specific complicated logic of our current MudList. I really hope that one day we will rewrite it that it's generic without two SelectedValue and SelectedItem.

[ScarletKuro]

I added this:

MudBlazor/src/MudBlazor.Docs/Pages/Components/List/Examples/ListSelectionExample.razor

Line 12 in f9eebe1

<MudListItem Text="Teas" InitiallyExpanded="true">

to test this logic:

MudBlazor/src/MudBlazor/Components/List/MudListItem.razor.cs

Line 277 in a53bcc1

await _expandedState.SetValueAsync(InitiallyExpanded);

And left it in the docs, I don't think it will hurt anyone (but let me know if I should remove).

Honestly, I don't really understand this InitiallyExpanded moment, why can't we just simply use two way binding and set the initial true for Expanded or set the Expanded to a constant (but my guess was because it was writing in own parameter and resting on re-render when it's constant but now it shouldn't be like this, because we handler is with ParameterState)

[henon]

I really hope that one day we will rewrite it that it's generic without two SelectedValue and SelectedItem.

Yes, we already discussed this with @mckaragoz for the upcoming v7.0 release as soon as he has won his elections, he'll start working on it.

[codecov]

Codecov Report

Attention: Patch coverage is 97.61905% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 88.80%. Comparing base (28bc599) to head (a53bcc1).
Report is 3 commits behind head on dev.

❗ Current head a53bcc1 differs from pull request most recent head 969425b. Consider uploading reports for the commit 969425b to get more accurate results

Files	Patch %	Lines
src/MudBlazor/Components/List/MudListItem.razor.cs	92.85%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #8297      +/-   ##
==========================================
- Coverage   89.82%   88.80%   -1.03%     
==========================================
  Files         412      405       -7     
  Lines       11878    12144     +266     
  Branches     2364     2419      +55     
==========================================
+ Hits        10670    10784     +114     
- Misses        681      832     +151     
- Partials      527      528       +1     

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

[henon]

Honestly, I don't really understand this InitiallyExpanded moment, why can't we just simply use two way binding and set the initial true for Expanded or set the Expanded to a constant (but my guess was because it was writing in own parameter and resting on re-render when it's constant but now it shouldn't be like this, because we handler is with ParameterState)

I guess we never thought of using two-way bindings for the problem because it is somewhat unintuitive that you have to create a field and bind it just for setting a default value. I believe in TreeViewItem we have a similar InitiallyExpanded property.

[ScarletKuro]

I guess we never thought of using two-way bindings for the problem because it is somewhat unintuitive that you have to create a field and bind it just for setting a default value. I believe in TreeViewItem we have a similar InitiallyExpanded property.

The

@bind-Expanded="_expanded"
bool _expanded = true;

Will work as for now out of the box and you don't need the InitiallyExpanded.

If you remove this line in code:

MudBlazor/src/MudBlazor/Components/List/MudListItem.razor.cs

Line 277 in a53bcc1

await _expandedState.SetValueAsync(InitiallyExpanded);

Then simple

Expanded="true"

will work too as if you set the InitiallyExpanded=true, and it doesn't reset on StateHasChanged since now we don't write to parameter directly like it was before.

[ScarletKuro]

Can we only do the nullable part in this PR? Then we can work on generic values firstly and will go step by step. It's (the value and item getter and setters) probably the most complicated part in whole library.

The nullable is just an addition to this PR, the point is to get rid of the BL0007 but keep the old behavior as before.
Technically this change shouldn't break codebeam-mudextensions if you are using MudList somewhere, at least I hope so.
I don't see the problem to merge it and work in next for the generic version it will be a breaking change, I also don't see problem to work in parallel if this PR will take longer than expected.

[henon]

Can we only do the nullable part in this PR? Then we can work on generic values firstly and will go step by step. It's (the value and item getter and setters) probably the most complicated part in whole library.

Yes, of course making list generic will be a separate PR to the v7 branch and we'll abolish the SelectedItem property as it won't be needed any longer

[henon]

Then simple

Expanded="true"

will work to as if you set the InitiallyExpanded=true, and it doesn't reset on StateHasChanged since now we don't write to parameter directly like it was before.

OK, let's make IsInitiallyExpanded obsolete with a message explaining that IsExpanded=true fill suffice from now on.

[ScarletKuro]

OK, let's make IsInitiallyExpanded obsolete with a message explaining that IsExpanded=true fill suffice from now on.

I'd love too, but we can't have both

1. If we want IsExpanded=true to work we need to remove:
   

   MudBlazor/src/MudBlazor/Components/List/MudListItem.razor.cs

   Line 277 in a53bcc1

   await _expandedState.SetValueAsync(InitiallyExpanded);

   
   Because it overrides the state, if we removing this line then whoever using IsInitiallyExpanded will get a breaking change since it won't be expanded anymore on IsInitiallyExpanded=true will be just a dummy parameter.
2. If we keep this line, then we can't leave a message with "please use IsExpanded=true" as it won't do anything because of that override on initialize. Only two way binding will work.

Maybe only if I make IsInitiallyExpanded as bool? and do some conditional logic, but idk if it can be breaking?

[henon]

Nah, don't worry for now, let's break the list in v7. It won't be long anyway

[ScarletKuro]

Closing this, since @henon working on the generic version.
Feel free to ping me for any help.

[henon]

Oh shit, I totally forgot you had already done this. I'd love to merge. Was this ready to merge?

[ScarletKuro]

Oh shit, I totally forgot you had already done this. I'd love to merge. Was this ready to merge?

Yes, it was
But now it has conflicts, let me see

[ScarletKuro]

Do not merge, i see that last example stopped to work after merge, tho all tests pass.

[ScarletKuro]

@henon actually it stopped to work even on https://dev.mudblazor.com so I guess it's latest regression that was commited.
And I think only ripple was?
When you go to the last example and start to change colors, they do not change in the list. If you compare it with https://mudblazor.com

[henon]

@ScarletKuro I think I'll directly merge your branch into mine and just work on fixing it.

[ScarletKuro]

I found the regression PR #8422 it's a massive one @danielchalmers
But currently don't understand what exactly causing the problem, i see the ListColorExample was changed.
When I move on one commit down it works fine. Does this means the @bind-SelectedValue & Value doesn't work correctly compared to the old @bind-SelectedOption & Option but wasn't it just a proxy methods.

[ScarletKuro]

Oh, I'm blind so the mistake is @bind-SelectedValue="_color" it should be @bind-Value="_color" without the Select in the begging. I don't know why razor doesn't give a compile error and I don't know how many more we have of such mistakes after refactor.

[ScarletKuro]

Done in a separate PR: #8606

[ScarletKuro]

@henon safe to merge now if you want, or just fetch it for your PR.

[henon]

Thanks!