Blazorise icon indicating copy to clipboard operation
Blazorise copied to clipboard

[Bug]: Auto Generate Columns conflict with dynamic columns

Open realLiangshiwei opened this issue 4 months ago • 10 comments

Blazorise Version

1.5.0

What Blazorise provider are you running on?

None

Link to minimal reproduction or a simple code snippet

@page "/"
@rendermode InteractiveServer

<PageTitle>Home</PageTitle>

<Button Color="Color.Primary" Clicked="AddColumns">Add dynamic columns</Button>

<DataGrid
    TItem="Book"
    Data="@Books"
    Responsive
    ShowPager
    ShowPageSizes>
    <DataGridColumns>
        @if (Columns != null)
        {
            @foreach (var column in Columns)
            {
                <DataGridColumn TItem="Book" Field="@column.Key" Caption="@column.Value">
                    
                </DataGridColumn>
            }
        }
    </DataGridColumns>
</DataGrid>


@code{

    public class Book
    {
        public string Name { get; set; }

        public string Author { get; set; }
    }
    
    List<Book> Books = new();
    Dictionary<string, string> Columns;

    Task AddColumns()
    {
        Columns = new Dictionary<string, string>
        {
            { nameof(Book.Name), "Name" },
            { nameof(Book.Author), "Author" }
        };

        return Task.CompletedTask;
    }

}

Steps to reproduce

  • Create a web app.
  • Copy and apply the simple code snippet
  • Click the add dynamic columns button
image

What is expected?

There should be no duplicate columns.

What is actually happening?

Auto Generate Columns conflict with dynamic columns

What browsers do you see the problem on?

No response

Any additional comments?

I think it's better not to automatically generate columns by default. Sometimes columns may be loaded dynamically, but by default the columns are generated automatically, after the columns are loaded dynamically there will be duplicate columns.

realLiangshiwei avatar Mar 27 '24 09:03 realLiangshiwei

This situation was found after ABP upgraded Blazorise to 1.5.0, and since ABP allows dynamic configuration of columns, we encountered duplicate columns.

image

realLiangshiwei avatar Mar 27 '24 09:03 realLiangshiwei

@stsrki Not wanting to be that "told you so" person, but I will be: image

Please advise since originally you asked for it to be magical, instead of opt in. https://github.com/Megabit/Blazorise/issues/3579


@realLiangshiwei As a workaround you can probably set some dummy invisible column to trick the system into not auto generating. image

David-Moreira avatar Mar 27 '24 09:03 David-Moreira

I still believe we should do the "magic" and assume that if no columns are defined, they should be autogenerated. In the users provided example, they use the TItem, and later, they manually add columns that are already in the IItem model. Maybe in this scenario, it makes more sense to hide or show columns by using the new ColumnChooser feature?

stsrki avatar Mar 27 '24 10:03 stsrki

Well I have to disagree. Because we want the magic, then the user has to do figure out how to work around it. I'm still of the original belief with the explicit opt in. Or at least able to turn it off.

Anyway, now we went this route, and we need to find sound solutions... :)

Maybe in this scenario, it makes more sense to hide or show columns by using the new ColumnChooser feature?

Can you further explain what this entails? The ColumnChooser is mainly an end user feature (the user that uses the UI). Are you suggesting that the programmer will hide them programmatically, or is the end user with the UI?

Let's stay, for example, I have 100 properties in the Item (I'm exagerating of course...), should I have to pay the cost for the 100 columns and then hide them?

David-Moreira avatar Mar 27 '24 10:03 David-Moreira

I'm still of the original belief with the explicit opt in. Or at least able to turn it off.

Yes, an option to turn it off would be useful.

realLiangshiwei avatar Mar 27 '24 10:03 realLiangshiwei

I mean to use ColumnChooser programmatically. But we only have that option in the next 1.6 with #5404. And it is done through the state management. So it is not possible at the moment.

We could introduce an opt-out API, but should we use it in the current 1.5?

@realLiangshiwei, did you try the workaround that David suggested?

stsrki avatar Mar 27 '24 11:03 stsrki

did you try the workaround that David suggested?

This is a workaround, and yes, it works. I just think this code is a bit ugly

realLiangshiwei avatar Mar 27 '24 13:03 realLiangshiwei

We could introduce an opt-out API, but should we use it in the current 1.5?

Well I would say that this is basically the AutoGenerateColumns initially proposed? Just that it's set to true by default. I don't see why not. It's flexibility without any real cost.

If you told me that adding the Parameter add any big downside... It seems like we are enforcing the "magic" just because we want to. Well we can enforce it by default, but let the user have something like AutoGenerateColumns so he can just do AutoGenerateColumns=false.

As usual I prefer to add it in v1.6 since there's indeed a workaround even though it's not super elegant. But the change is so simple it wouldn't hurt to do it in v1.5, again, I personally just don't like new apis popping up mid release if not absolutely necessary.

So go ahead and let me know if you finally agree with the Parameter, or if you have any other idea, and what version would you prefer to do this on.

David-Moreira avatar Mar 27 '24 22:03 David-Moreira

You have convinced me. Do the change in 1.5, and if it is not too big, once I review, then it will be merged in.

The API will be AutoGenerateColumns and, by default, is enabled.

stsrki avatar Mar 28 '24 09:03 stsrki

@David-Moreira reminder

stsrki avatar Apr 09 '24 12:04 stsrki