MudBlazor icon indicating copy to clipboard operation
MudBlazor copied to clipboard

MudList<T>: New Experimental List Implementation

Open mckaragoz opened this issue 3 years ago • 10 comments

Description

Big List and Select PR #5150 has been decided to seperate multiple phases. The big PR describes what it has, so we didn't write it here again.

MudList, MudSelect and MudAutocomplete will remain the same. Instead of we will add the new components with a new namespace MudExperimental. So both of the old and remaked components can be usable and testable by developers during the phases.

Please note that these experimental components are not planned to be changed drastically unless unexpected bugs are encountered during the phases. It totally depends on the success rate.

Roadmap

Phase 1: Experimental List. (Current Phase) Creating MudExperimental.MudList

Phase 2: Old List Arrangement Bug fixes which are proper for old design for old MudList.

Phase 3: Experimental Select Creating MudExperimantal.MudSelect

Phase 4: Old Select Arrangement Bug fixes which are proper for old design for old MudSelect.

Phase 5: Experimental Autocomplete Creating MudExperimantal.MudAutocomplete

Phase 6: Old Autocomplete Arrangement Bug fixes which are proper for old design for old MudAutocomplete.

Phase 7: Overall Evaluation and Final Decision Phase.

Phase Summary

Will be added as a comment in this PR.

mckaragoz avatar Sep 26 '22 22:09 mckaragoz

Awesome! Just a random idea: How about merging the features of MudAutocomplete and MudSelect into a new Mud... component, since they can be similar.

Yomodo avatar Sep 26 '22 23:09 Yomodo

Awesome! Just a random idea: How about merging the features of MudAutocomplete and MudSelect into a new Mud... component, since they can be similar.

I think thats not be possible, because autocomplete doesn't use public Items parameter. It process all item's within its SearchFunc. They still have their base differencies.

I also think seperate select and autocomplete is familiar for most developers.

mckaragoz avatar Sep 27 '22 13:09 mckaragoz

Codecov Report

Attention: Patch coverage is 76.28319% with 134 lines in your changes missing coverage. Please review.

Project coverage is 90.83%. Comparing base (f18e217) to head (89b4644). Report is 2094 commits behind head on dev.

Files with missing lines Patch % Lines
...azor/Experimental/Components/List/MudList.razor.cs 74.94% 110 Missing :warning:
.../Experimental/Components/List/MudListItem.razor.cs 80.89% 17 Missing :warning:
...zor/Experimental/Components/List/MudListItem.razor 73.91% 6 Missing :warning:
...dBlazor/Experimental/Components/List/MudList.razor 75.00% 1 Missing :warning:

:x: Your patch check has failed because the patch coverage (76.28%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #5368      +/-   ##
==========================================
- Coverage   91.38%   90.83%   -0.55%     
==========================================
  Files         371      371              
  Lines       14770    15204     +434     
==========================================
+ Hits        13497    13811     +314     
- Misses       1273     1393     +120     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

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

codecov[bot] avatar Sep 27 '22 17:09 codecov[bot]

To get the review process going I am starting with a quick summary of the API with screenshots and comments. This PR is hard to review because in addition to the change to <T> there are so many new things that each will need discussion. I hope we can manage somehow.

API review

Ignore spelling or English problems at the moment, I'll fix those later. Let's discuss the API of the new MudList<T> first:

image This is how it looks like with the MudExperimental namespace. Quite verbose (I know, I suggested this ;) ). But we might want to do something less intrusive, like X for experimental or instead of using a namespace just use a different prefix than Mud like Mux. How does MuxList look? Not bad I think:

<MuxList T="string">
  <MuxListItem T="string" Text="Single List Item" SecondaryText="Secondary Text" />
  <MuxListItem T="string" Text="Single List Item" SecondaryText="Secondary Text" />
</MuxList>

Apart from the change to T this is the second most important API change/improvement. image

            <MudExperimental.MudList ItemCollection="states" Clickable="true" MaxItems="_maxItems" />
            <MudExperimental.MudList T="Continent" ItemCollection="(ICollection<Continent>)Enum.GetValues(typeof(Continent))" Clickable="true" MaxItems="_maxItems" />
@code
{
    int _maxItems = 8;

    private string[] states =
    {
        "Alabama", "Alaska", "American Samoa", "Arizona",
// [...] snipped for brevity
        "Washington", "West Virginia", "Wisconsin", "Wyoming",
    };

    public enum Continent
    {
        Europe,
        Asia,
        America,
        Africa,
        Australia,
        Antarctica
    }
}

I gotta say this is sweet! Makes me wonder if we even need or want inline item definition any more. This would be so much more powerful with a good set of RenderFragments. Only the parameter MaxItems needs to be improved. This is actually a height, not a max number of items, so maybe MaxHeightItems ??? but I am also not sure about that.

image Here we immediately see the problem the duality of inline items and item collection. Some parameters only apply to the one or the other, which I think increases confusion and complexity in the API. Below we'll see lots of specialized new parameters which are only there because there is no way to customize the item layout with render fragements.

image I think we might want to rename parameter SecondaryBackgroundForNestedItemHeader

image Note the new feature SecondaryText. We'll probably want to change DisablePadding and DisableGutters. First of all, we have a new policy to name things positively not negatively (i.e. EnablePadding) but in this case maybe a Padding="0" (disabled) vs Padding="4" (enabled) would be better API? Maybe also a width in px for Gutter(s)? Not yet sure what Gutter is supposed to be in any case so that should be clarified also. Is it just an extra padding to the left?

image image

<MudExperimental.MudList @ref="_list" T="int?" Clickable="_clickable" MultiSelection="_multiSelection" SelectAll="_selectAll" @bind-SelectedValue="_selectedValue" @bind-SelectedItem="_selectedItem" @bind-SelectedValues="_selectedValues" @bind-SelectedItems="_selectedItems">
	<MudExperimental.MudListItem T="int?" Text="Sparkling Water (1)" Value="1" />
	<MudExperimental.MudListItem T="int?" Text="Still Water (2)" Value="2" />
	<MudExperimental.MudListItem T="int?" Text="Teas" InitiallyExpanded="true">
	    <NestedList>
		<MudExperimental.MudListItem @ref="_thirdItem" T="int?" Text="Earl Grey (3)" Value="3" />
		<MudExperimental.MudListItem T="int?" Text="Matcha (4)" Value="4" />
		<MudExperimental.MudListItem @ref="_fifthItem" T="int?" Text="Pu'er (5)" Value="5" />
	    </NestedList>
	</MudExperimental.MudListItem>
	<MudExperimental.MudListItem T="int?" Text="Coffees">
	    <NestedList>
		<MudExperimental.MudListItem T="int?" Text="Irish Coffee (6)" Value="6" />
		<MudExperimental.MudListItem T="int?" Text="Double Espresso (7)" Value="7" />
		<MudExperimental.MudListItem T="int?" Text="Cafe Latte (8)" Value="8" />
	    </NestedList>
	</MudExperimental.MudListItem>
</MudExperimental.MudList>

Here it is to be discussed what to do with Clickable and MultiSelection. I am not entirely sure if we shouldn't simply do a SelectionMode="SelectionMode.None" vs SelectionMode="SelectionMode.SingleSelection" vs SelectionMode="SelectionMode.MultiSelection". The variant of not being able to select the item body and only select the checkbox might not even be needed. I would find such a list confusing.

This example can be improved. I might work on that. The huge advantage of the new list is that we can set the value type (int? in this case). So instead of doing things with @ref it should be done by changing the selected integer value(s).

image

<MudExperimental.MudList T="string" Clickable="true" MultiSelection="true" MultiSelectionAlign="_listAlign"
                     MultiSelectionComponent="_listSelectionComponent" DisableSelectedItemStyle="_disableSelectedItemStyle">
	<ChildContent>
	    <MudExperimental.MudListItem Value="@("Wifi")" Text="Wifi" Icon="@Icons.Filled.Wifi" />
	    <MudExperimental.MudListItem Value="@("Bluetooth")" Text="Bluetooth" Icon="@Icons.Filled.Bluetooth" />
	    <MudDivider />
	    <MudExperimental.MudListItem Value="@("Location")" Text="Location" Icon="@Icons.Filled.LocationOn" />
	    <MudExperimental.MudListItem Value="@("High Security")" Text="High Security" Icon="@Icons.Filled.Security" />
	</ChildContent>
</MudExperimental.MudList>

Here we want to think about the naming of parameters MultiSelectionComponent, MultiSelectionAlign and DisableSelectedItemStyle and if we even want the parameter DisableSelectedItemStyle. Maybe via CSS with SelectedItemClass would be a better approach? As a minor side-note, when using AlignEnd there is too much padding on the right side. Also I can't shake the feeling that instead of MultiSelectionComponent a RenderFragment might be better.

Let's also in general think about what other features can be made more configurable with RenderFragments instead of limited boolean or enum parameters.

sticky subheaders

             <MudExperimental.MudList T="string" Clickable="true" MultiSelection="true" Style="max-height:400px; overflow-y: auto;" DisablePadding="_disablePadding">
                <MudExperimental.MudListSubheader T="string" Sticky="_sticky" SecondaryBackground="_secondaryBackground">England</MudExperimental.MudListSubheader>
                @foreach (string club in _englishClubs)
                {
                    <MudExperimental.MudListItem Value="@club" Text="@club" />
                }

                <MudExperimental.MudListSubheader T="string" Sticky="_sticky" SecondaryBackground="_secondaryBackground">Spain</MudExperimental.MudListSubheader>
                @foreach (string club in _spanishClubs)
                {
                    <MudExperimental.MudListItem Value="@club" Text="@club" />
                }

                <MudExperimental.MudListSubheader T="string" Sticky="_sticky" SecondaryBackground="_secondaryBackground">Germany</MudExperimental.MudListSubheader>
                @foreach (string club in _germanClubs)
                {
                    <MudExperimental.MudListItem Value="@club" Text="@club" />
                }

                <MudExperimental.MudListSubheader T="string" Sticky="_sticky" SecondaryBackground="_secondaryBackground">Turkey</MudExperimental.MudListSubheader>
                @foreach (string club in _turkishClubs)
                {
                    <MudExperimental.MudListItem Value="@club" Text="@club" />
                }
            </MudExperimental.MudList>

I'll not even say much about this. I feel like this should have been a separate PR to be able to discuss it in depth. But obviously a group header RenderFragment would be a much better solution.

image This is the same as old list.

image Last but not least, this is also new and a good feature I think. The only problem I see is that with all items selected there is no way to tell where I am navigating with the up and down keys. image

henon avatar Oct 16 '22 19:10 henon

I gotta say this is sweet! Makes me wonder if we even need or want inline item definition any more. This would be so much more powerful with a good set of RenderFragments. Only the parameter MaxItems needs to be improved. This is actually a height, not a max number of items, so maybe MaxHeightItems ??? but I am also not sure about that.

Maybe MaxVisibleItems or MaxShownItems. But im not a fan of leaving renderfragment approach especially on list. The list is the base component and it should support all possible design options. Maybe we can discuss it for select and autocomplete, but not for list i think. I feel we can support both approaches without going too hacky and complex, and it will be a sign of Mud that shows our power and customizability.

And MaxItems parameter gives the list a simple height CSS, so if the list item is custom and has different height value, the parameter doesn't work properly. Maybe we need to add one more parameter to define custom list item heght if user want.

Here we immediately see the problem the duality of inline items and item collection. Some parameters only apply to the one or the other, which I think increases confusion and complexity in the API. Below we'll see lots of specialized new parameters which are only there because there is no way to customize the item layout with render fragements.

Yes, virtualization works only for ItemCollection approach, but it can be easily added in renderfragment approach. Actually users can simply copy the base code what we did in MuxList.

Here it is to be discussed what to do with Clickable and MultiSelection. I am not entirely sure if we shouldn't simply do a SelectionMode="SelectionMode.None" vs SelectionMode="SelectionMode.SingleSelection" vs SelectionMode="SelectionMode.MultiSelection". The variant of not being able to select the item body and only select the checkbox might not even be needed. I would find such a list confusing.

This is one of the most important design question for me. The current one has Clickable and Selectable status. When users set clickable parameter to true it doesn't mean the list also can selectable. And there is no seperate selectable parameter, users have to add @bind-SelectedItem parameter to make list selectable. This is very confusing and we doesn't use this like approach anywhere on the library. So firstly we changed the approach that now Clickable now also means "Selectable", much more simple. And as i explain on the example we certainly need the 4 different states about selection and multiselection. Im open to suggestion a new API that includes the all 4 states.

Here we want to think about the naming of parameters MultiSelectionComponent, MultiSelectionAlign and DisableSelectedItemStyle and if we even want the parameter DisableSelectedItemStyle. Maybe via CSS with SelectedItemClass would be a better approach? As a minor side-note, when using AlignEnd there is too much padding on the right side. Also I can't shake the feeling that instead of MultiSelectionComponent a RenderFragment might be better.

Im in favor on making renderfragment and also have the default fragment to show if the renderfragment is null. But we need to pass the "Checked" parameter into MultiSelectionComponent renderfragment to control the two different state design.

And padding of the right side may be caused by MudSwitch itself. Need to examine again and if the problem source is switch, lets fix it.

I'll not even say much about this. I feel like this should have been a separate PR to be able to discuss it in depth. But obviously a group header RenderFragment would be a much better solution.

I thought i fixed the overlap problem, but it seems i reverted something and its background color lost again.

Last but not least, this is also new and a good feature I think. The only problem I see is that with all items selected there is no way to tell where I am navigating with the up and down keys.

Ah yes, i forgot to think about it. maybe we should add alpha value (some transparency) to selected background color.


On conclusion, i agree we should have renderfragments and default designs for them (if they are null) as much as possible, because its a base component.

mckaragoz avatar Oct 16 '22 22:10 mckaragoz

About the namespace for experimental environment, what about MudX. ?

HClausing avatar Oct 17 '22 12:10 HClausing

Any updates on this one? 😊

dennisrahmen avatar Jan 20 '23 08:01 dennisrahmen

Any updates on this one? 😊

Still working 🙂 I hope will announce something soon.

mckaragoz avatar Jan 20 '23 17:01 mckaragoz

There is 1 good and 1 bad news. Start with bad one:

  • The experimental MudList will not add into core library in short period.

And the good one:

  • MudListExtended is now ready on MudExtensions. The extended component has all features that described here.

Ready on Nuget: https://www.nuget.org/packages/CodeBeam.MudExtensions/6.2.0 Try online: https://codebeam-mudextensions.pages.dev/mudlistextended

mckaragoz avatar Jan 28 '23 17:01 mckaragoz

Now MudSelectExtended is also released.

Ready on Nuget: https://www.nuget.org/packages/CodeBeam.MudExtensions/6.2.1 Try online: https://codebeam-mudextensions.pages.dev/mudselectextended

mckaragoz avatar Feb 03 '23 19:02 mckaragoz

This pull request has been marked as stale.

Please let us know how we can help move it forwards! @MudBlazor/triage

github-actions[bot] avatar Jul 19 '25 05:07 github-actions[bot]