Fix ParameterState & Converter usage #592
Conversation
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (69.76%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## dev #592 +/- ##
==========================================
+ Coverage 67.92% 67.98% +0.05%
==========================================
Files 105 105
Lines 7415 7408 -7
Branches 1280 1275 -5
==========================================
- Hits 5037 5036 -1
+ Misses 1900 1899 -1
+ Partials 478 473 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| /// </summary> | ||
| protected MudBaseInputExtended() | ||
| { | ||
| Converter = new DefaultConverter<T> |
There was a problem hiding this comment.
No need, MudBaseInput already sets a GetDefaultConverter() => DefaultConverter<T>
| OnDebounceIntervalElapsed="@(async() => await OnDebounceIntervalElapsed.InvokeAsync())" | ||
| OnInternalInputChanged="@(async() => await OnInternalInputChanged.InvokeAsync())" | ||
| ShrinkLabel="@(Values?.Any() ?? false)" | ||
| ShrinkLabel="@(_valuesState.Value?.Any() ?? false)" |
There was a problem hiding this comment.
Rule of parameter state. Always use the backing field, no exceptions
| await MudComboBox.ToggleOption(this, !Selected); | ||
| await InvokeAsync(StateHasChanged); | ||
| await MudComboBox.FocusAsync(); | ||
| if (MudComboBox is not null) |
There was a problem hiding this comment.
I saw that in many places MudComboBox has a null check but not here, and for some reason the MudComboBox wasn't declared as null possible, but as null!, you need to decide here. Can it be null or not? If not then you can remove the null checks everywhere.
| @if (ButtonVariant == ButtonVariant.Button) | ||
| { | ||
| <MudButton Disabled="@(Disabled ? true : _loading.Value ? true : false)" | ||
| <MudButton Disabled="@(Disabled || (_loading.Value))" |
There was a problem hiding this comment.
Same just simplified.
We don't do with booleans _loading.Value ? true : false just straight _loading.Value as is already either true or false.
| field.Find("input").Input(new ChangeEventArgs() { Value = "sdfg" }); | ||
| await comp.InvokeAsync(() => comp.Instance.HandleBeforeInput(new BeforeInputEventArgs() { Data = " ", InputType = "insert" })); | ||
| comp.Instance.Values.Should().BeEquivalentTo(new List<string> { "asdf", "asd", "sdfg" }); | ||
| comp.Instance.GetState(x => x.Values).Should().BeEquivalentTo(new List<string> { "asdf", "asd", "sdfg" }); |
There was a problem hiding this comment.
Should use GetState when you accessing parameter state value from other places, otherwise the value might be incorrect.
|
@mckaragoz feel free to reject some changes, as I did some cosmetics one too. You can make ur own PR based on this. |
|
Thanks, thats bunch of work and seems a great PR, i will look at it. Some components written 4 years ago, these ones do not share same quality with new ones. I will handle all components one by one and want AI to review also (between Mud 9 and 10). |
|
And yes don't trust the tests these are most of basic render tests and can only catch initial null references etc. Only some components like select and stepper has real tests. |
src/CodeBeam.MudBlazor.Extensions/Components/ComboBox/MudComboBox.razor.cs
Show resolved
Hide resolved
|
Thanks 🥇 |
2 participants
Please, test the components.
I tested visually but I don't know the specifics how the components should behave and the current website is down.
Tests are also green, but there are only 170 tests which isn't enough.
P.S. Don't use
base.XXXunless you overload the method and know you you need the default behavior.You used the base.ConvertSet, but when you overload it, now you are calling the base one instead of your custom one, as result you need to change everywhere.