Skip to content

MudPagination: Use ParameterState#10154

Merged
henon merged 7 commits intoMudBlazor:devfrom
ScarletKuro:pager
Nov 1, 2024
Merged

MudPagination: Use ParameterState#10154
henon merged 7 commits intoMudBlazor:devfrom
ScarletKuro:pager

Conversation

@ScarletKuro
Copy link
Copy Markdown
Member

@ScarletKuro ScarletKuro commented Oct 31, 2024

Description

Let's make this component better

How Has This Been Tested?

Unit tests are working and visually checked

Type 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)
  • Documentation (fix or improvement to the website or code docs)

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 Unexpected behavior or functionality not working as intended PR: needs review labels Oct 31, 2024
@codecov
Copy link
Copy Markdown

codecov bot commented Nov 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.24%. Comparing base (56ef312) to head (5162fa0).
Report is 3 commits behind head on dev.

Additional details and impacted files
@@           Coverage Diff           @@
##              dev   #10154   +/-   ##
=======================================
  Coverage   91.23%   91.24%           
=======================================
  Files         411      411           
  Lines       12479    12492   +13     
  Branches     2432     2434    +2     
=======================================
+ Hits        11385    11398   +13     
  Misses        552      552           
  Partials      542      542           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ScarletKuro ScarletKuro requested a review from henon November 1, 2024 00:06
@ScarletKuro
Copy link
Copy Markdown
Member Author

Btw, I’ve always wondered why we have three pagination components: MudDataGridPage, MudTablePager, and MudPagination. Why not just use the generic MudPagination? I understand that the first two have dropdowns and other features, plus they include CascadingParameter for direct access to the component. However, we could include all of that into MudPagination and create a common interface for components that define CurrentPage, RowsPerPage, GetFilteredItemCount, etc.
But that's off-topic.

@ScarletKuro ScarletKuro added breaking change This change will require consumer code updates (ex: removes/changes a public API) v8 labels Nov 1, 2024
@henon
Copy link
Copy Markdown
Contributor

henon commented Nov 1, 2024

Question is always if it's worth it. I have had negative experience with such refactorings because you couple components with each other which otherwise would be completely independent. It could be easier to break one or the other whenever changing the shared component or at least create more headache whenever a change is necessary in one but not the other etc. Hard to say, you might be right and make things simpler or you could make things worse.

Comment thread src/MudBlazor/Components/Pagination/MudPagination.razor.cs
@henon henon merged commit 2b54ce6 into MudBlazor:dev Nov 1, 2024
@henon
Copy link
Copy Markdown
Contributor

henon commented Nov 1, 2024

Thanks @ScarletKuro!

@ScarletKuro ScarletKuro deleted the pager branch November 1, 2024 20:21
@ScarletKuro
Copy link
Copy Markdown
Member Author

ScarletKuro commented Nov 1, 2024

Question is always if it's worth it. I have had negative experience with such refactorings because you couple components with each other which otherwise would be completely independent. It could be easier to break one or the other whenever changing the shared component or at least create more headache whenever a change is necessary in one but not the other etc. Hard to say, you might be right and make things simpler or you could make things worse.

You wouldn't really couple them. The DataGrid/Table wouldn't need to know anything about the Pagination component. All it would need to do is implement an interface that provides information such as the current number of items, the current page, and the total number of pages, they both already have this info. The MudPagination wouldn't be aware of them as well, it would simply listen for the CascadingParameter. If it's present, it could manipulate those properties, similar to how MudDataGridPager and MudTablePager operate. If the CascadingParameter is null (i.e., not inside the PagerContent of a component), it would function as a standalone paginator, just like the current MudPagination work already.

Additionally, we could provide a RenderFragment<Context> for customization. If this isn't sufficient, users could still replace it with their own implementation. For example, in a table, you can replace the MudTablePager with MudPagination using the ref, or they could also listen for the CascadingParameter since the interface would be public.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change This change will require consumer code updates (ex: removes/changes a public API) bug Unexpected behavior or functionality not working as intended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants