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

MudExpansionPanel: Use ParameterState, remove obsolete API and change to async API #8446

Merged
merged 3 commits into from
Mar 25, 2024

Conversation

ScarletKuro
Copy link
Member

@ScarletKuro ScarletKuro commented Mar 24, 2024

Description

Fixes: #8429

Breaking Changes:

Remove IsInitiallyExpanded

It's not necessary anymore.
You can either set a constant

<MudExpansionPanels MultiExpansion>
    <MudExpansionPanel Class="panel-one" Text="Panel One" IsExpanded="true">
        ...
    </MudExpansionPanel>
</MudExpansionPanels>

StateHasChanged will not reset on re-render back to constant because we avoided writing to the Parameter https://learn.microsoft.com/en-us/aspnet/core/blazor/components/overwriting-parameters?view=aspnetcore-8.0

or you can use two way binding.

<MudExpansionPanels MultiExpansion>
    <MudExpansionPanel Class="panel-one" Text="Panel One" @bind-IsExpanded="_isExpanded">
        ...
    </MudExpansionPanel>
</MudExpansionPanels>

@code
{
    private bool _isExpanded = true;
}

To set the initial expansion.

Change API to be async.

This component was firing EventCallbacks in a fire-and-forget manner. We changed all the API to be async.

Simplify code

We do not need _collapseIsExpanded variable or updateParent arguments, since the ParamterState manages the state correctly, therefore everything can be done in a more simple way.

How Has This Been Tested?

Visual, current unit tests, new unit test.

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.

@ScarletKuro ScarletKuro requested a review from henon March 24, 2024 18:41
@github-actions github-actions bot added breaking change bug Something does not work as intended/expected PR: needs review labels Mar 24, 2024
@ScarletKuro ScarletKuro added the API change API that needs approval label Mar 24, 2024
Copy link

codecov bot commented Mar 24, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 88.86%. Comparing base (61e8a76) to head (027833e).

Files Patch % Lines
...mponents/ExpansionPanel/MudExpansionPanel.razor.cs 81.81% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #8446      +/-   ##
==========================================
+ Coverage   88.84%   88.86%   +0.01%     
==========================================
  Files         416      416              
  Lines       12358    12307      -51     
  Branches     2458     2447      -11     
==========================================
- Hits        10980    10937      -43     
+ Misses        845      836       -9     
- Partials      533      534       +1     

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

@henon
Copy link
Collaborator

henon commented Mar 24, 2024

I don't understand why we need the IExpansionPanelManager interface at all. Why not simply use internal methods like before? If I understand correctly, this change doesn't concern the users at all as the interface is internal anyway.

@henon
Copy link
Collaborator

henon commented Mar 24, 2024

Using only IsExpandedChanged is considered invalid now.

If you only care about getting notifications I think this should still be valid usage?

@henon henon mentioned this pull request Mar 24, 2024
@ScarletKuro
Copy link
Member Author

ScarletKuro commented Mar 24, 2024

I don't understand why we need the IExpansionPanelManager interface at all. Why not simply use internal methods like before? If I understand correctly, this change doesn't concern the users at all as the interface is internal anyway.

True 😂 I just saw it was using event so I automatically put an interface because I got this observer-observable pattern in my head, I will change that.

@ScarletKuro
Copy link
Member Author

ScarletKuro commented Mar 24, 2024

If you only care about getting notifications I think this should still be valid usage?

It will not work currently if you just subscribe only to IsExpandedChanged and do not update the IsExpanded, it will not collapse or expand (depending on the initial IsExpanded). At first I thought everything is working as intended and it only worked because we modified IsExpanded directly and when i set it in the component this effect disappeared, but now after debugging I realised it's a ParamterState framework bug 😤. I know how to fix it, but I need to come up with integration tests since we didnt catch this bug before and it's not something you can unit tests as you need the re-renders etc.

@ScarletKuro
Copy link
Member Author

ScarletKuro commented Mar 25, 2024

Fixed: #8457
Rebased this branch, reverted the example with just IsExpandedChanged, and changed the PR description.
I removed the IsExpandedChanged moment in the #8447
Not making public the ParamterState for now was a good idea.

@ScarletKuro ScarletKuro merged commit ab608c2 into MudBlazor:dev Mar 25, 2024
3 checks passed
peterthorpe81 pushed a commit to peterthorpe81/MudBlazor that referenced this pull request Mar 25, 2024
@ScarletKuro ScarletKuro deleted the expand_state branch March 25, 2024 15:12
biegehydra pushed a commit to biegehydra/MudBlazor that referenced this pull request Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API change API that needs approval breaking change bug Something does not work as intended/expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MudExpansionPanels inconsistent behavior when binding IsExpanded
2 participants