Treeview templating improvements

[tungi52]

@Codinkat has a few additional suggestions in #495, but it has been merged before I process them. This PR contains the following changes:

• created a reusable MudTreeViewItemArrow component
• the content renderfragment in MudTreeItem will replace the entire content, not only Text parameter
• when CanActivate is true, the cursor changes to pointer over the tree item
• updated complex example
• add test for templated treeview

@Codinkat can you check it? I leave this as draft until then.

[Codinkat]

Yeah, absolutly love it!!

I think we have here a really great component, with very useful properties for common scenarios, yet easy to completely customize ;-)

I just noticed some minor things:

1-I suggest to define the arrow as the default mouse cursor. Actually, when the mouse is over a "standard" node (ie, just text, not clickable, etc.), the mouse cursor is the "text" cursor, which seems to me not very natural (because the node text is not an editable text, like a TextField)

2-The doc should have some more details about the use of the CanActivate property. It seems to be a kind of "single-select", is it? If this is the case, maybe you could add an other component beside the tree to show how to bind to the currently selected (activated) node.

3-Same for CanSelect: pretty cool to allow multiple-selection, but I suggest to show how to bind to this selection.

4-Custom Tree example: I suggest to remove the <style> element, and use instead the predefined custom classes from MudBlazor to adjust the margins:

<MudIcon Icon="@context.Icon" Class="ml-0 mr-2" Color="@Color.Default" />

5-Custom Tree example: I suggest to add a notice to explain the behavior of the Content property, something like this:

When Content property is used, it will completely replace the default rendering of the MudTreeViewItem to use your own

Great work!

[tungi52]

@Codinkat Thanks for your detailed feedback! I'm going to change them.

[tungi52]

@Codinkat I didn't know there are such classes: ml-0 mr-2 I have to dig much deeper in this project :) Thanks again!

[Codinkat]

@tungi52 About the bindings (point 2 & 3), to be more precise, I think some properties are missing on the MudTree component:

1-"ActiveItem" (MudTreeViewItem type); should of course manage the deactivation of the previous activated item if setter is receiving a new one ;-)

2-"SelectedItems" (Hashset type). I propose HashSet because it is what is used for MudSelect.

Some other small things I just noticed inside the code:

-I think the cascading parameter MudTreeRoot of MudTree could be private, since it seems to be an implementation detail.

-Since one can easily customize the rendering of the Item by using the Content property, IconContent and EndIconContent (which act more as some placeholders) seems to be unnecessary now.

[tungi52]

@Codinkat I updated my code, I hope I didn't miss anything. I didn't remove IconContent and EndIconContent, because I think, it could be real-life scenario when someone want to use custom icons (for example font awesome) but don't want to customize the tree entirely.

[Garderoben]

@tungi52 tell me when you feel ready for merge.

[Codinkat]

@tungi52 Ok, I'll take a look soon ;-)

I understand your point about the IconContent and EndIconContent properties, but I think this is not a very common scenario... I think the best is to check what is done in other components of the library, so I searched for "EndIcon" and it returned two components (Button and Menu), and a search for "IconContent" returned nothing... This is why I personally don't think there is a real need for this feature at current time, but it is only my 2 cents ;-)

Maybe @henon could also give us some input about this (was it requested in the past, etc)

[henon]

We have established a pattern for string properties like icons, texts, captions etc which we are adding more and more throughout the library, namely, adding a renderfragment property with suffix Content which overrides the simple property on demand.

Example from MudExpansionPanel:

string Title {get; set; } and RenderFragment TitleContent

In MessageBox I did this for every of the string properties Title, Message, YesText, etc.

The short string properties are very handy because they can be assigned as attribute. But more often than not we discovered, that users wanted to style that text or add icons or whatever. Thus we are adding the ...Content options wherever possible to allow that.

The rule is if render fragment is null show the simple property, otherwise the RF overrides, no matter the value of the simple property.

Sorry, I didn't have time to dive into this PR in depth. I just added my two cents, which you guys may be aware of already.

[Codinkat]

Good to know!

Thanks @henon

[Codinkat]

@tungi52 I checked the updated code and you missed nothing ;-)

The only thing I see now is the IsActivated management at the TreeViewItem level in the complex example. My understanding is that you wanted to control the activated item by the data object, but the current code makes things very strange and in a server scenario, using a static variable to keep track of the activated item will fail because all app clients will use the same variable reference, which will of course fail.

After a look to the code of TreeViewItem, I notice that the properties having a corresponding "Changed" event does not raise the event in the setter. For example:

[Parameter]
public bool Activated { get; set; }

[Parameter]
public EventCallback<bool> ActivatedChanged { get; set; }

...if a new activated state is set by the Activated property setter, the ActivatedChanged event is not raised (same for Expanded, Selected, etc) so the bound variable will not receive the new value. Maybe this is a hint? (Even if not, events should be raised when properties are changed by their setter ;-)

[tungi52]

@Codinkat You are right, making a static field was a huge mistake, I changed it. Well, I don't see a problem on that the Changed event are not called in the setters. Internally (when someone click in the checkbox, activate an item) events are handled in a separate method, and there the changed events are raised. These property setters (I think) will only called through the bindings, but in case I don't see this as a problem. On the other hand, you are right about that though the properties are updated, some 'dependent' values are not. For example if I activate an item via binding, the ActivatedItem in the TreeView won't. I fixed that.

[Codinkat]

Well, because the xxxChanged is a public trigger to say to the external world "hey, this value has changed", not raising the event in the setter (also public) is a break in the observer pattern, and as you said, some content may then not react correctly to state changes in some scenarios.

[tungi52]

@Codinkat Understand. I changed it.

[tungi52]

@Codinkat I try to explain a bit more detailed what I thought about (and this could attributable to my limited knowledge about Blazor). I think we don't have here a classic observer pattern, because EventCallbacks in blazor are single-cast events, not multi-casted, as in classic .NET. So an EventCallback has only one target. In my previous code, TreeViewItem if wanted to notify, it calls XXXChanged, so the binding target will know about it and changes its value. So if the property's setter called from 'inside', we are ok, because event is raised from elsewhere. What remains, is when setter called from outside, through the binding. In this case I haven't seen a benefit to call this event, because the sender should already know about it. I can think only one possible problematic scenario:

<MudTreeViewItem @bind-Activated="Activated" @bind-Activated:event="ActivatedChanged" />

And you are right, in this case ActivatedChanged probably won't called when I change the value of Activated. But for me, this is a bit unreal.

I don't know if I'm right, please correct me!

[tungi52]

@Codinkat One more thing: If I write for example:

treeViewItem.Expanded = true;

I get a warning, that component parameter should not be set outside of its component.

[mikes-gh]

@tungi52 I guess you worked it out. You are not allowed to set component parameters outside of a component because a re-render will overwrite and give you an indeterminate state. Use callbacks or shared state or cascading parameters.

[tungi52]

@mikes-gh Yes I know :) I didn't have a problem with it. This was just for an additional thought to my previous comment.

[Codinkat]

@tungi52 The Observer pattern does not limit to multicast implementations. As soon as an object can register to an other object to be notified about a state change, it is the observer pattern ;-)

@mikes-gh You are right, the compiler throws a warning when we attempt to modify directly the property. But this does not mean that Blazor does not use it directly... In fact, it does.

When you bind two components on the same variable (let's say two checkbox on the same bool variable), if the component property does not raise its event, the change will not be propagated to the other component. I am currently working on this exact case on a MudBlazor component ;-)

[tungi52]

@Codinkat Ah, ok, I see now. This is not what I think about in the first place, but yes, you are absolutely right. Anyway, I updated my code earlier. I don't know what to do with IconContent and EndIconContent, but maybe it is not a big deal if the component has two more parameters.

[Codinkat]

@tungi52 What do you mean about IconContent and EndIconContent? You are wondering if you should leave them or remove them?

[tungi52]

@Codinkat I read your above comment again, and you convinced me. I removed those two parameters.

[Codinkat]

@tungi52 In the complex example, I see that you wish to demonstrate that it is possible to bind to a variable which will hold the activated data object (and not the activated tree view item component). This can be done is a simpler way if the binding for the Activated property of the MudTreeViewItem is done using the Changed event, like this:

Activated="@context.IsActivated" ActivatedChanged="() => ActivatedItem = context"

As you see, this allows to store directly the context object, so it is no longer needed to do this in the setter of the IsActivated property of CustomTreeItem. The IsActivated becomes then a simple get/set property, and this also makes no longer necessary to pass down the root object.

So, the final code is:

@namespace MudBlazor.Docs.Examples

<MudTreeView CanActivate="true" CanSelect="true" Style="width: 500px;">
    <MudTreeViewItemBuilder Items="TreeItems">
        <MudTreeViewItem Activated="@context.IsActivated" ActivatedChanged="() => ActivatedItem = context" @bind-Expanded="@context.IsExpanded" @bind-Selected="@context.IsSelected" Tag="@context"
                         Icon="@context.Icon" Text="@context.Title" EndText="@context.Number" EndTextTypo="@Typo.caption" Items="@context.TreeItems" />
    </MudTreeViewItemBuilder>
</MudTreeView>

<div style="width: 100%">
    <MudText Typo="@Typo.subtitle1">Activated item: @ActivatedItem?.Title</MudText>
    <MudText Typo="@Typo.subtitle1">Number of selected items: @GetSelectedItems(TreeItems).Count()</MudText>
</div>

@code { 
    class CustomTreeItem
    {
        public string Title { get; set; }

        public string Icon { get; set; }

        public string Number { get; set; }

        public bool IsActivated { get; set; }

        public bool IsSelected { get; set; }

        public bool IsExpanded { get; set; }

        public List<CustomTreeItem> TreeItems { get; set; }

        public CustomTreeItem(string title, string icon, string number = null)
        {
            Title = title;
            Icon = icon;
            Number = number;
        }
    }

    CustomTreeItem ActivatedItem { get; set; }

    List<CustomTreeItem> TreeItems { get; set; } = new List<CustomTreeItem>();

    public TreeViewComplexExample()
    {
        TreeItems.Add(new CustomTreeItem("All Mail", Icons.Material.Email));
        TreeItems.Add(new CustomTreeItem("Trash", Icons.Material.Delete));
        TreeItems.Add(new CustomTreeItem("Categories", Icons.Material.Label)
        {
            IsExpanded = true,
            TreeItems = new List<CustomTreeItem>()
            {
                new CustomTreeItem("Social", Icons.Material.Group, "90"),
                new CustomTreeItem("Updates", Icons.Material.Info, "2,294"),
                new CustomTreeItem("Forums", Icons.Material.QuestionAnswer, "3,566"),
                new CustomTreeItem("Promotions", Icons.Material.LocalOffer, "733")
            }
        });
        TreeItems.Add(new CustomTreeItem("History", Icons.Material.Label));
    }

    static IEnumerable<CustomTreeItem> GetSelectedItems(List<CustomTreeItem> treeItems)
    {
        foreach (var treeItem in treeItems)
        {
            if (treeItem.IsSelected)
                yield return treeItem;

            foreach (var selected in GetSelectedItems(treeItem.TreeItems))
            {
                yield return selected;
            }
        }
    }
}

[Codinkat]

@tungi52 For the complex example, I think it would be interesting to show that a powerful technique is to attach the data object to the TreeViewItem component by using the Tag property.

In this example, I use the ActivatedItem and SelectedItems properties of the MudTreeView so I don't have to manage this myself. But since I still have access to the associated data object, I can display the Title and even compute the sum of the selected data objects:

@namespace MudBlazor.Docs.Examples

<MudTreeView CanActivate="true" CanSelect="true" @bind-ActivatedItem="ActivatedItem" @bind-SelectedItems="SelectedItems">
    <MudTreeViewItemBuilder Items="TreeItems">
        <MudTreeViewItem @bind-Expanded="@context.IsExpanded" 
                         Icon="@context.Icon" Text="@context.Title" EndText="@context.Number?.ToString()" EndTextTypo="@Typo.caption" 
                         Items="@context.TreeItems" Tag="@context" />
    </MudTreeViewItemBuilder>
</MudTreeView>

<div style="width: 100%">
    <MudText Typo="@Typo.subtitle1">Activated item: @((ActivatedItem?.Tag as CustomTreeItem)?.Title)</MudText>
    <MudText Typo="@Typo.subtitle1">Sum of selected items: @GetSelectedSum()</MudText>
</div>

@code { 
    class CustomTreeItem
    {
        public string Title { get; set; }

        public string Icon { get; set; }

        public int? Number { get; set; }

        public bool IsExpanded { get; set; }

        public List<CustomTreeItem> TreeItems { get; set; }

        public CustomTreeItem(string title, string icon, int? number = null)
        {
            Title = title;
            Icon = icon;
            Number = number;
        }
    }

    MudTreeViewItem ActivatedItem { get; set; }

    HashSet<MudTreeViewItem> SelectedItems { get; set; }

    List<CustomTreeItem> TreeItems { get; set; } = new List<CustomTreeItem>();

    public TreeViewComplexExample()
    {
        TreeItems.Add(new CustomTreeItem("All Mail", Icons.Material.Email));
        TreeItems.Add(new CustomTreeItem("Trash", Icons.Material.Delete));
        TreeItems.Add(new CustomTreeItem("Categories", Icons.Material.Label)
        {
            IsExpanded = true,
            TreeItems = new List<CustomTreeItem>()
            {
                new CustomTreeItem("Social", Icons.Material.Group, 90),
                new CustomTreeItem("Updates", Icons.Material.Info, 2294),
                new CustomTreeItem("Forums", Icons.Material.QuestionAnswer, 3566),
                new CustomTreeItem("Promotions", Icons.Material.LocalOffer, 733)
            }
        });
        TreeItems.Add(new CustomTreeItem("History", Icons.Material.Label));
    }

    public int GetSelectedSum()
    {
        return SelectedItems?.Select(i => (CustomTreeItem)i.Tag).Sum(i => i.Number ?? 0) ?? 0;
    }
}

Also, I think that this example is really about presenting the Builder component and showing how to efficiently use it. So, what about renaming the title to "TreeViewItem Builder" with the following description?

This example shows how to use the MudTreeViewItemBuilder to automatically build the tree items according to a hierarchical data structure.

If so, I would suggest to rename the "CustomTreeItem" class in the example to "TreeItemData" ;-)

[tungi52]

@Codinkat For me, it is ok. I updated the example.

[henon]

Is this done?

[Codinkat]

@henon I tried it with the last updated examples. I think we have now a pretty good release candidate ;-)

[tungi52]

@Codinkat Thank you very much! @Garderoben @henon I mark this as ready for review.

[henon]

Thank you @tungi52 for your awesome contributions. Thanks to @Codinkat and all other reviewers for helping out. All efforts are greatly appreciated!

[Codinkat]

@henon you're welcome. #493 can be closed now!