Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DataGrid Reading Twice When non-default PageSize changes #3610

Closed
TorreyGarland opened this issue Mar 18, 2022 · 13 comments
Closed

DataGrid Reading Twice When non-default PageSize changes #3610

TorreyGarland opened this issue Mar 18, 2022 · 13 comments
Assignees
Labels
Type: Bug 🐞 Something isn't working
Milestone

Comments

@TorreyGarland
Copy link

TorreyGarland commented Mar 18, 2022

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:

  • blazorise.bootstrap5 0.9.5.6
  • blazorise.charts 0.9.5.6
  • blazorise.components 0.9.5.6
  • blazorise.datagrid 0.9.5.6
  • blazorise.fluentvalidation 0.9.5.3
  • blazorise.icons.fontawesome 0.9.5.6
  • blazorise.richtextedit 0.9.5.6
  • blazorise.sidebar 0.9.5.6
  • blazorise.spinkit 0.9.5.6

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 TorreyGarland added the Type: Bug 🐞 Something isn't working label Mar 18, 2022
@TorreyGarland
Copy link
Author

TorreyGarland commented Mar 18, 2022

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

@stsrki stsrki added this to the 1.0 Support milestone Mar 18, 2022
@David-Moreira
Copy link
Contributor

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
Copy link
Author

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
Copy link
Author

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
Copy link
Contributor

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

@David-Moreira
Copy link
Contributor

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
Copy link
Contributor

David-Moreira commented Mar 19, 2022

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
Copy link
Author

TorreyGarland commented Mar 19, 2022

@bind-page="_pageSize" works.

is there a bindable property for "CurrentPage"?

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

@stsrki
Copy link
Collaborator

stsrki commented Mar 19, 2022

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
Copy link
Collaborator

stsrki commented Mar 21, 2022

Scheduling the work for v1.1.

@stsrki stsrki modified the milestones: 1.0 Support, 1.1 Mar 21, 2022
@David-Moreira
Copy link
Contributor

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
Copy link
Collaborator

stsrki commented Mar 31, 2022

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
Copy link
Contributor

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.

@stsrki stsrki closed this as completed Dec 19, 2024
@github-project-automation github-project-automation bot moved this from No Status to Done in Development Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug 🐞 Something isn't working
Projects
Status: Done
Development

No branches or pull requests

4 participants