Skip to content

MudDataGrid: Fix grouping for bound and unbound scenarios using ParameterState#8463

Merged
ScarletKuro merged 2 commits intoMudBlazor:devfrom
peterthorpe81:fix/muddatagrid-grouping-binding
Apr 1, 2024
Merged

MudDataGrid: Fix grouping for bound and unbound scenarios using ParameterState#8463
ScarletKuro merged 2 commits intoMudBlazor:devfrom
peterthorpe81:fix/muddatagrid-grouping-binding

Conversation

@peterthorpe81
Copy link
Contributor

@peterthorpe81 peterthorpe81 commented Mar 25, 2024

Fixes MudDataGrid column grouping so it works for bound and unbound scenarios.

Description

Fixes: #8159
This fix implements the ParameterState framework for MudDataGrid column grouping. Allowing the parameter to be used in a bound state or as an initializer as disussed in #8258

There is a breaking change (minor I believe) in relation to the Groupable parameter. If you set grouping in code to true and groupable to false it will render grouped. Similarly if you @bind-grouping it won't respect the groupable parameter. Previously the behaviour was inconsistent between the initial value and an updated parameter. The grid UI fully respects the groupable parameter as before.

I think this is appropriate behaviour, the UI enforces the groupable parameter but the developer has the choice to ignore it and group by any column. I can't think of a scenario where this will be an issue. There is potentially a way to retain the behaviour but it does mean in @bind-grouping scenarios with groupable="false" there is an immediate event callback to reset grouping. It would seem less flexible to me anyway.

How Has This Been Tested?

Tested visually

Added test DataGridGroupingTestBoundAndUnboundScenarios to try grouping for bound and unbound scenarios and expected behaviour.

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 breaking change This change will require consumer code updates PR: needs review labels Mar 25, 2024
@codecov
Copy link

codecov bot commented Mar 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.38%. Comparing base (0fe4f81) to head (c1122f1).

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #8463      +/-   ##
==========================================
- Coverage   89.39%   89.38%   -0.02%     
==========================================
  Files         413      413              
  Lines       11865    11863       -2     
  Branches     2354     2353       -1     
==========================================
- Hits        10607    10604       -3     
- Misses        736      737       +1     
  Partials      522      522              

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

@ScarletKuro
Copy link
Member

Thanks for the PR, I will look it later!

@ScarletKuro ScarletKuro self-requested a review March 25, 2024 14:02
@ScarletKuro ScarletKuro self-assigned this Mar 25, 2024
@ScarletKuro ScarletKuro force-pushed the fix/muddatagrid-grouping-binding branch from 8a13de7 to 27bc9f6 Compare April 1, 2024 00:06
@ScarletKuro
Copy link
Member

LGTM.
Tho not sure if we should pollute example with all this one way / two way binding combinations, but I guess I will leave it for now, we can cleanup if needed.

PS: I rebased against latest change, to make sure we don't get formatting errors.

@ScarletKuro ScarletKuro added bug Unexpected behavior or functionality not working as intended and removed PR: needs review labels Apr 1, 2024
@ScarletKuro
Copy link
Member

Hmmm seems like some build errors after rebase, I will investigate later.

@ScarletKuro
Copy link
Member

@Anu6is
your added tests fail
3eee937

[Test]
    public void DateRangePicker_Preset_No_Timestamp()
    {
        var comp = Context.RenderComponent<DateRangePickerPresetWithoutTimestampTest>();

        comp.Markup.Should().Contain("mud-range-start-selected");
        comp.Markup.Should().Contain("mud-range-end-selected");
    }

    [Test]
    public void DateRangePicker_Preset_Timestamp()
    {
        var comp = Context.RenderComponent<DateRangePickerPresetRangeWithTimestampTest>();

        comp.Markup.Should().Contain("mud-range-start-selected");
        comp.Markup.Should().Contain("mud-range-end-selected");
    }

problem
The mud-range-start-selected is not visible because its on the previous month.

@Anu6is
Copy link
Contributor

Anu6is commented Apr 1, 2024

Corrected unit tests in #8543

@ScarletKuro ScarletKuro force-pushed the fix/muddatagrid-grouping-binding branch from 27bc9f6 to c1122f1 Compare April 1, 2024 12:16
@ScarletKuro ScarletKuro changed the title MudDataGrid fix grouping for bound and unbound scenarios using ParameterState MudDataGrid: Fix grouping for bound and unbound scenarios using ParameterState Apr 1, 2024
@ScarletKuro ScarletKuro merged commit 605bf92 into MudBlazor:dev Apr 1, 2024
@ScarletKuro
Copy link
Member

@peterthorpe81 thanks.

biegehydra pushed a commit to biegehydra/MudBlazor that referenced this pull request Apr 26, 2024
@peterthorpe81 peterthorpe81 deleted the fix/muddatagrid-grouping-binding branch June 26, 2024 23:15
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 bug Unexpected behavior or functionality not working as intended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MudDataGrid - Grouping should be bindable

3 participants