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

MudDataGrid: Fix filtering modes when using ServerData #6627

Merged
merged 9 commits into from Apr 12, 2023
Merged

MudDataGrid: Fix filtering modes when using ServerData #6627

merged 9 commits into from Apr 12, 2023

Conversation

ghost
Copy link

@ghost ghost commented Apr 11, 2023

Description

Fixes ServerData not reloaded when using FilterMode ColumnFilterMenu or ColumnFilterRow regression in #6555

How Has This Been Tested?

Unit and visually

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • The PR is submitted to the correct branch (dev).
  • My code follows the code style of this project.
  • I've added relevant tests.

@github-actions github-actions bot added bug Something does not work as intended/expected PR: needs review labels Apr 11, 2023
@ghost ghost mentioned this pull request Apr 11, 2023
6 tasks
@ghost ghost marked this pull request as draft April 11, 2023 13:07
Copy link
Collaborator

@henon henon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. @alfadelta88 Is it ready for merge from your point of view?

@codecov
Copy link

codecov bot commented Apr 11, 2023

Codecov Report

Patch coverage: 53.84% and project coverage change: +0.12 🎉

Comparison is base (49e627a) 90.63% compared to head (ed2db51) 90.76%.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #6627      +/-   ##
==========================================
+ Coverage   90.63%   90.76%   +0.12%     
==========================================
  Files         399      399              
  Lines       13587    13582       -5     
==========================================
+ Hits        12315    12328      +13     
+ Misses       1272     1254      -18     
Impacted Files Coverage Δ
src/MudBlazor/Components/DataGrid/Filter.cs 77.96% <0.00%> (ø)
...dBlazor/Components/DataGrid/FilterHeaderCell.razor 37.50% <0.00%> (ø)
src/MudBlazor/Components/DataGrid/HeaderCell.razor 89.65% <ø> (ø)
...rc/MudBlazor/Components/DataGrid/MudDataGrid.razor 88.33% <ø> (ø)
src/MudBlazor/Components/DataGrid/FilterContext.cs 54.54% <25.00%> (-16.89%) ⬇️
...azor/Components/DataGrid/FilterHeaderCell.razor.cs 42.85% <45.45%> (+13.94%) ⬆️
.../MudBlazor/Components/DataGrid/HeaderCell.razor.cs 71.92% <50.00%> (+3.44%) ⬆️
...MudBlazor/Components/DataGrid/MudDataGrid.razor.cs 90.71% <90.00%> (+0.57%) ⬆️

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ghost
Copy link
Author

ghost commented Apr 11, 2023

@henon there is one small annoyance with the filter when using ColumnFilterMenu that goes like this open filter menu -> input value -> apply a filter -> server called -> open filter menu -> remove value -> grid refreshes with all the items without calling server

@henon
Copy link
Collaborator

henon commented Apr 11, 2023

I see now that you marked it as draft.

@ghost
Copy link
Author

ghost commented Apr 11, 2023

I'm working on it right now, I'll expand the tests when I fix it.

@ScarletKuro
Copy link
Member

ScarletKuro commented Apr 11, 2023

The current solution seems acceptable to me, but I'm concerned about the number of InvokeServerLoadFunc / ReloadServerData calls spreader everywhere in the code , which is quite high. It would be great if we could improve the design in the future and introduce something like IMudDataGridChangedNotification (args type of change filter / sort / pagination etc). This would trigger data reload in a single place.

@ghost
Copy link
Author

ghost commented Apr 11, 2023

@ScarletKuro you're absolutely right, it's repetitive and error prone and it will only get more difficult to debug.

@ghost ghost marked this pull request as ready for review April 11, 2023 13:43
@ghost
Copy link
Author

ghost commented Apr 11, 2023

Fixed it, silly multiple enumerations mistakes in tests. @henon Ready for review :)

@ScarletKuro ScarletKuro requested a review from henon April 11, 2023 13:44
@ScarletKuro
Copy link
Member

silly multiple enumerations mistakes

This is also concerning, and I noticed this problem before as well, there are a lot of multiple enumerations in some places, and there are places where IEnumerable doesn't make sense. But the problem is that a lot of such places are public API.

@ghost
Copy link
Author

ghost commented Apr 11, 2023

silly multiple enumerations mistakes

This is also concerning, and I noticed this problem before as well, there are a lot of multiple enumerations in some places, and there are places where IEnumerable doesn't make sense. But the problem is that a lot of such places are public API.

Exactly, ReSharper or Rider would help greatly with this issue although they don't catch'em all.

@ScarletKuro
Copy link
Member

I will tag @tjscience as well.

@dennisrahmen
Copy link
Contributor

@henon if I am not mistaken this is now ready.

Copy link
Collaborator

@henon henon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, good PR. Thanks a lot!

@henon henon merged commit 33c2493 into MudBlazor:dev Apr 12, 2023
2 of 3 checks passed
@tjscience
Copy link
Contributor

Looks good. Thanks!

@ghost ghost deleted the fix/muddatagrid-serverdata-filter-modes branch April 12, 2023 20:34
ilovepilav pushed a commit to ilovepilav/MudBlazor that referenced this pull request Nov 25, 2023
MudDataGrid: Fix filtering modes when using ServerData (MudBlazor#6627)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something does not work as intended/expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants