MudBlazor / MudBlazor

Blazor Component Library based on Material design with an emphasis on ease of use. Mainly written in C# with Javascript kept to a bare minimum it empowers .NET developers to easily debug it if needed.
http://mudblazor.com
MIT License
7.22k stars 1.18k forks source link

Standardise the use of `Visible` #8832

Closed BieleckiLtd closed 1 week ago

BieleckiLtd commented 2 weeks ago

MudBlazor currently uses the Visible and IsVisible properties. This PR aims to standardise the use of Visible.

Description

If this PR is approved, the v7 migration guide must also be updated, as this makes a breaking change:

MudCarouselItem: replace IsVisible with Visible ChartSeries: replace IsVisible with Visible SvgLegend: replace IsVisible with Visible MudDialog: replace IsVisible with Visible MudDialog: replace IsVisibleChanged with VisibleChanged MudMessageBox: replace IsVisible with Visible MudMessageBox: replace IsVisibleChanged with VisibleChanged MudTooltip: replace IsVisible with Visible MudTooltip: replace IsVisibleChanged with VisibleChanged

Linked issues: Negative property names should be discouraged #6131 v7.0.0 Migration Guide #8447

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

Type of Changes

Checklist

codecov[bot] commented 2 weeks ago

Codecov Report

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

Project coverage is 90.13%. Comparing base (28bc599) to head (d91a0c0). Report is 142 commits behind head on dev.

Files Patch % Lines
...lazor/Components/MessageBox/MudMessageBox.razor.cs 20.00% 4 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev #8832 +/- ## ========================================== + Coverage 89.82% 90.13% +0.30% ========================================== Files 412 421 +9 Lines 11878 12304 +426 Branches 2364 2431 +67 ========================================== + Hits 10670 11090 +420 + Misses 681 665 -16 - Partials 527 549 +22 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

henon commented 2 weeks ago

@BieleckiLtd I wanted to merge this but I noticed that we probably should always also add the eventcallback to the illegal param detector for the cases where people didn't use the @bind- syntax. I.e. IsVisibleChanged. You could also add the event callbacks for your other standardization PRs, i.e. IsCheckedChanged etc.

BieleckiLtd commented 2 weeks ago

@BieleckiLtd I wanted to merge this but I noticed that we probably should always also add the eventcallback to the illegal param detector for the cases where people didn't use the @bind- syntax. I.e. IsVisibleChanged. You could also add the event callbacks for your other standardization PRs, i.e. IsCheckedChanged etc.

I did add IsCheckedChanged but you might be right about IsVisibleChanged. I will double check.

BieleckiLtd commented 1 week ago

Added IsVisibleChanged to DetectIllegalRazorParametersV7 and updated migration guide:

MudDialog: replace IsVisibleChanged with VisibleChanged MudMessageBox: replace IsVisibleChanged with VisibleChanged MudTooltip: replace IsVisibleChanged with VisibleChanged

henon commented 1 week ago

Thanks @BieleckiLtd . Do you think we need to standardize other things as well or should we call it a day for v7?

Oh, I see you were planning on doing IsDisabled, IsSelected, IsActive and IsValid as well.

henon commented 1 week ago

@BieleckiLtd I think we may want to leave IsValid as is in IForm and MudForm as EditForm and FluentValidation both use IsValid and if we change MudForm to Valid it would actually increase inconsistency instead of decreasing it.

BieleckiLtd commented 1 week ago

I'd like to do IsSelected and IsActive over the weekend. I believe IsDisabled was changed to Enabled, but I'll double check.

henon commented 1 week ago

Great, I am changing IsTouched to Touched as we also have Touched already.

Edit: I decided to leave IsTouched alone also. We might want to change it again after reworking form in v8 so it doesn't make much sense to touch it now.