Megabit / Blazorise

Blazorise is a component library built on top of Blazor with support for CSS frameworks like Bootstrap, Tailwind, Bulma, AntDesign, and Material.
https://blazorise.com/
Other
3.25k stars 528 forks source link

DataGrid Reading Twice When non-default PageSize changes #3610

Open TorreyGarland opened 2 years ago

TorreyGarland commented 2 years ago

Describe the bug DataGrid "ReadData" event is called twice when it's PageSize property is set to a non-default value; in this case 10;

The datagrid currently only has a callback for the ReadData event for server-side sorting, filtering and pagination. DataGrid data is only populated from the ReadData callback.

This does not happen with the component page first loads - only after the value from the page size drop down is changed.

This also does not happen if PageSize is not set in markup.

StateHasChanged or InvokeAsync(StateHasChanged) are never explicitly invoked.

To Reproduce Steps to reproduce the behavior:

  1. Navigate to a page with a Blazorise DataGrid component, PageSize set to 10 - ReadData fires once as expected.
  2. Change the value in the page size dropdown from 10 to 5.
  3. Callback for ReadData fire twice. Two database calls instead of one.

Expected behavior ReadData callback fires one time if PageSize dropdown value is changed.

Notes This a Blazor Server App, not Web Assembly.

Target Framework: net6.0

Visual Studio Enterprise 2022, version 17.0.4

All current .NET SDKs

Several custom source generators such as thisassembly installed.

Nuget Packages:

Render mode for the app is set to "Server" and not "ServerPrerendered" in _Host.cshtml because of too many issues of callbacks firing twice instead of once on initial page load.

Example Code Markup Code

<DataGrid TItem="Item" 
                  Data="Data" 
                  PageSize="10" /* Issue only occurs if this is explicitly set */
                  ReadData="ReadData" 
                  ShowPager
                  ShowPageSizes 
                  Responsive
                  TotalItems="Data?.TotalItemCount">
</DataGrid>

Code Behind

private IPagedList<Item>? Data;

// called once on page load
// called twice if page size changes.
private async Task ReadData(DataGridReadDataEventArgs<Item> e)
{
    Data = await Service.GetPagedListAsync(e.Page, e.PageSize, e.CancellationToken).ConfigureAwait(false);
}

BTW, this is one of the best packages that I have ever used. Please keep up the good work.

TorreyGarland commented 2 years ago

most applicable packages updated to version 1.0.1 today. Read data still firing twice as described.

David-Moreira commented 2 years ago

Hello @TorreyGarland Glad you're liking our package.

I am unable to reproduce the issue with our demo in rel-1.0 branch. I made it so the definition is pretty close to yours:

                <DataGrid @ref="dataGrid"
                          TItem="Employee"
                          ReadData="@OnReadData"
                          Data="@employeeList"
                          TotalItems="@totalEmployees"
                          ShowPager
                          ShowPageSizes
                          Responsive
                          PageSize="10">

But only calls it once. Can you please provide a repository with the issue? Would really appreciate it, Thanks!

TorreyGarland commented 2 years ago

Link

https://gist.github.com/TorreyGarland/2d66f761431554e827f7213e0b3ffa9a

this is my first time using Gist.

One thing I forgot to add is that I was using ReactiveUI.Blazor but when I created the issue originally the ViewModel logic had nothing to do with ReadData.

TorreyGarland commented 2 years ago

I also notice the same behavior if a value is inserted for "CurrentPage"

<DataGrid TItem="WeatherForecast" 
          Data="ViewModel!.Forecasts"
          DetailRowStartsVisible="false"
          Responsive
          ReadData="(e) => ViewModel!.Reload.Execute(e)"
          PageSizeChanged="(pageSize) => { }"
          Resizable
          PageSize="10"
          ShowPageSizes
          ShowPager
          CurrentPage="2"
          Sortable="false"
          HeaderThemeContrast="ThemeContrast.Dark"
          TotalItems="50">
    <DataGridColumns>
        <DataGridColumn TItem="WeatherForecast"
                        Field="@nameof(WeatherForecast.Date)"
                        Caption="Date"
                        DisplayFormat="{0:d}" />
        <DataGridColumn TItem="WeatherForecast"
                        Field="@nameof(WeatherForecast.TemperatureC)"
                        Caption="Temp. (C)" />
        <DataGridColumn TItem="WeatherForecast"
                        Field="@nameof(WeatherForecast.TemperatureF)"
                        Caption="Temp. (F)" />
        <DataGridColumn TItem="WeatherForecast"
                        Field="@nameof(WeatherForecast.Summary)"
                        Caption="Summary" />
    </DataGridColumns>
    <LoadingTemplate>
        <Paragraph Italic>Loading...</Paragraph>
    </LoadingTemplate>
</DataGrid>

I thought that maybe creating an empty callback for "PageSizedChanged" might work, but it doesn't.

David-Moreira commented 2 years ago

Well, would have been better straight up repo, I'll create a new project and copy paste the stuff. Thanks

David-Moreira commented 2 years ago

Hello, Still unable to reproduce by copying your gist. Can you take a look at our repository and submit the necessary changes to reproduce the issue? Would really appreciate it, Thanks!

https://github.com/Megabit/DataGridTest

David-Moreira commented 2 years ago

OH wait!! I was just re-reading the steps to reproduce, I was testing by refreshing and other datagrid operations...

So your problem is by changing page sizes. Well you should not show the PageSize Selection(ShowPageSizes) if you're hardcoding a PageSize, the DataGrid will just reload back to the hardcoded PageSize. If you want to set an initial page size but still have the value be changed, then you need to hold the value in a variable, like this:

@bind-PageSize="_pageSize"
private int _pageSize = 10;

This way Blazor can mutate the value successfully, otherwise it's just an hardcoded value. Please let us know if these changes help you. :)

TorreyGarland commented 2 years ago

@bind-page="_pageSize" works.

is there a bindable property for "CurrentPage"?

Setting CurrentPage="_currentPage" where _currentPage = 2 fires twice also.

stsrki commented 2 years ago

It seems we forgot to adjust this API. At the moment it is not bindable, but you can make the same thing with the manual callback.

CurrentPage="@currentPage"
PageChanged="@(e=> currentPage = e.Page)"

@David-Moreira we should change the CurrentPage API in v2.0 to: Page/PageChanged

stsrki commented 2 years ago

Scheduling the work for v1.1.

David-Moreira commented 2 years ago

It seems we forgot to adjust this API. At the moment it is not bindable, but you can make the same thing with the manual callback.

CurrentPage="@currentPage"
PageChanged="@(e=> currentPage = e.Page)"

@David-Moreira we should change the CurrentPage API in v2.0 to: Page/PageChanged

So I was starting this, and I noticed we have : [Parameter] public EventCallback<DataGridPageChangedEventArgs> PageChanged { get; set; }

We'd need to be changing it to [Parameter] public EventCallback<int> PageChanged { get; set; }

Does that make sense at this point? I don't think the args make much sense, it only brings in the extra pagesize info, which in my opinion adds nothing to this callback, and we're loosing the @bind-Page functionality which is probably much more needed.

Otherwise we could maintain CurrentPage and go with [Parameter] public EventCallback<int> CurrentPageChanged { get; set; } instead.

However I'd advocate to make it cleaner and just remove the PageChanged DataGridPageChangedEventArgs in favor of int

stsrki commented 2 years ago

That means it would be a breaking change and we would have to move it to 2.0. I think DataGridPageChangedEventArgs was a first attempt at trying to read external APIs so it doesn't make much sense today, now that we have ReadData callback. But as I said we can't change it until 2.0

David-Moreira commented 2 years ago

Better just re-schedule for 2.0 then. No point in doing anything here I guess.. EVen introducing Page beforehand would probably just be more confusing.