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

Standardise the use of Expanded, Expandable, IsExpanded and IsExpandable #8718

Merged
merged 5 commits into from
Apr 20, 2024

Conversation

BieleckiLtd
Copy link
Contributor

@BieleckiLtd BieleckiLtd commented Apr 16, 2024

MudBlazor currently uses the Expanded, Expandable, IsExpanded and IsExpandable properties. This PR aims to standardise the use of Expanded and Expandable.

Description

#6131, #8447,
If this PR is approved, the v7 migration guide must also be updated, as this makes a breaking change.

Standardise the use of IsEnabled and Enabled #8764
Standardise the use of ItemDisabled #8887
Standardise the use of Checked, CheckedChanged and Checkable #8825
Standardise the use of Visible #8832
Standardise the use of Selected and SelectedChanged #8886
Standardise the use of Expanded, Expandable, IsExpanded and IsExpandable #8718
Standardise the use of Active #8888
Standardise the use of Open and OpenChanged #8891
Standardise the use of Editable #8892
Standardise the use of Hidden and HiddenChanged #8952

How Has This Been Tested?

unit | visually

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 breaking change bug Something does not work as intended/expected PR: needs review labels Apr 16, 2024
Copy link
Contributor

@danielchalmers danielchalmers left a comment

Choose a reason for hiding this comment

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

We're definitely leaning toward Expanded over IsExpanded because it's closer to web standards. See recent PRs: https://github.com/MudBlazor/MudBlazor/pulls?q=is%3Apr+author%3Ahenon+rename

@ScarletKuro
Copy link
Member

ScarletKuro commented Apr 16, 2024

introduces ParameterState to MudNavGroup

I would do in a separate PR since there are nuances, and it would be easier for me to explain in a smaller PR.

@BieleckiLtd BieleckiLtd changed the title Standardise the use of Expanded and IsExpanded Standardise the use of Expanded, Expandable, IsExpanded and IsExpandable Apr 16, 2024
Copy link

codecov bot commented Apr 16, 2024

Codecov Report

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

Project coverage is 90.09%. Comparing base (28bc599) to head (7836b63).
Report is 84 commits behind head on dev.

Files Patch % Lines
src/MudBlazor/Components/Table/MudTable.razor.cs 0.00% 0 Missing and 1 partial ⚠️
...MudBlazor/Components/Table/TableGroupDefinition.cs 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #8718      +/-   ##
==========================================
+ Coverage   89.82%   90.09%   +0.26%     
==========================================
  Files         412      419       +7     
  Lines       11878    12175     +297     
  Branches     2364     2396      +32     
==========================================
+ Hits        10670    10969     +299     
+ Misses        681      667      -14     
- Partials      527      539      +12     

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

@ScarletKuro ScarletKuro requested a review from henon April 16, 2024 20:31
@henon
Copy link
Collaborator

henon commented Apr 17, 2024

@BieleckiLtd thank you for helping! I agree with the proposed changes.

This is gonna be conflicted by #8661 which totally overhauls treeview. Hope it won't be too much work to resolve conflicts.

Could you prepare a list of changes in the format of

  • MudExpansionPanel: replace IsExpanded with Expanded
  • ...

That would help me a lot, when integrating with the migration guide.

Finally, because Razor doesn't raise errors with parameters that don't exist, it is necessary to do a Find in Files to check for any remaining instances of IsEx.+. I take it you did that?

There are quite a few other parameters starting with Is prefix, I guess we should get rid of all of them in separate PRs. Let me know if you want to do those too.

@henon henon added API change API that needs approval v7 New major MudBlazor version labels Apr 17, 2024
@BieleckiLtd
Copy link
Contributor Author

BieleckiLtd commented Apr 18, 2024

This is gonna be conflicted by #8661

I will wait for #8661 to be merged then then pull dev into my branch and resolve conflicts. Setting this as a draft for now.

Could you prepare a list of changes

Ok.

parameters that don't exist, it is necessary to do a Find in Files to check for any remaining instances of IsEx.+. I take it you did that?

Yes, I did.

There are quite a few other parameters starting with Is prefix, I guess we should get rid of all of them in separate PRs. Let me know if you want to do those too.

That would make sense if we're breaking things. I can work on them on a case by case basis like this PR. Few that I have found are
Standardise the use of IsEnabled and Enabled #8764
IsDisabled
IsChecked
IsVisible
IsSelected
Standardise the use of Expanded, Expandable, IsExpanded and IsExpandable #8718
IsActive
IsValid

@BieleckiLtd BieleckiLtd marked this pull request as draft April 18, 2024 09:09
@BieleckiLtd
Copy link
Contributor Author

MudExpansionPanel: replace IsExpanded with Expanded
MudExpansionPanel: replace IsExpandedChanged with ExpandedChanged
GroupDefinition: replace IsExpandedChanged with ExpandedChanged
MudTHeadRow: replace IsExpandable with Expandable
MudTFootRow: replace IsExpandable with Expandable
MudTableGroupRow: replace IsExpanded with Expanded
MudTr: replace IsExpandable with Expandable

@BieleckiLtd BieleckiLtd marked this pull request as ready for review April 20, 2024 13:17
@henon henon merged commit 04e143c into MudBlazor:dev Apr 20, 2024
4 checks passed
@henon
Copy link
Collaborator

henon commented Apr 20, 2024

@BieleckiLtd Thanks, this is big help!

@henon
Copy link
Collaborator

henon commented Apr 20, 2024

Added to v7.0.0 Migration Guide #8447

@BieleckiLtd BieleckiLtd deleted the fix/standardise-expanded branch April 20, 2024 17:15
biegehydra pushed a commit to biegehydra/MudBlazor that referenced this pull request Apr 26, 2024
…Expandable` (MudBlazor#8718)

Co-authored-by: Pawel Bielecki <Pawel.Bielecki2@leoni.com>
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 v7 New major MudBlazor version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants