MudForm: Add ReadOnly and Disabled properties #6511
Conversation
|
|
||
| if (formControl is IReadOnlyDisabledFormComponent component) //automaticlaly set the readonly and disabled state | ||
| { | ||
| component.ReadOnly = ReadOnly || (ParentMudForm?.ReadOnly).GetValueOrDefault(); |
There was a problem hiding this comment.
Could be refactored to ?? false
|
|
||
| namespace MudBlazor.Interfaces | ||
| { | ||
| public interface IReadOnlyDisabledFormComponent : IFormComponent |
| foreach (var control in _formControls.Where(x => x is IReadOnlyDisabledFormComponent)) | ||
| { | ||
| ((IReadOnlyDisabledFormComponent)control).ReadOnly = _readOnly; | ||
| control.InternalStateHasChanged(); |
There was a problem hiding this comment.
This StateHasChanged call is necessary to ensure the controls accept their new state. Alternatively, the MudForm could be cascaded with Fixed="false" but in my testing this broke a number of other tests.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## dev #6511 +/- ##
==========================================
+ Coverage 91.37% 91.38% +0.01%
==========================================
Files 397 397
Lines 14919 14930 +11
==========================================
+ Hits 13632 13644 +12
+ Misses 1287 1286 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
|
If someone sets ReadOnly manually, the form will override the setting which isn't desirable. I need to make some changes to avoid that. |
|
For the MudForm? You can implement them explicitly too i guess. If someone casts to interface he gets the access, but idk why anyone would do that. |
|
@ScarletKuro I meant that if someone set ReadOnly or Disabled on an individual form control, it would be completely overridden by the MudForm. The MudForm ReadOnly/Disabled defaults to false so all child components will ignore their own ReadOnly/Disabled parameters. |
|
Yes, I agree. Individual parameters should always override general parent parameters. |
|
Ok, i got it now, but I'd say it depends. |
|
I guess that would be plausible behavior too |
|
I set individual form controls to read only based on previously selected values. Then when saving or updating data I would like to set the hole form to read only regardless of the state of individual components. Maybe when setting read only on the form to 'null' it would use the individual form control settings and 'true' or 'false' would override them? |
|
@ScarletKuro I like this idea and it would be easy to implement using or: Your logic above boils down to if either the parent or child have Alternatively as @dennisrahmen said the parent properties could be nullable and not be overridable when set. |
|
Additionally, as there can be nested forms, I think the override behavior should be reflected in how nested forms behave. |
| if (Disabled || (ParentMudForm?.Disabled).GetValueOrDefault() != _disabled) //only run if the disabled state has changed | ||
| { | ||
| _disabled = Disabled || (ParentMudForm?.Disabled).GetValueOrDefault(); | ||
| foreach (var control in _formControls.Where(x => x is IReadOnlyDisabledFormComponent)) |
There was a problem hiding this comment.
Can also write it like this
foreach (var control in _formControls)
{
if (control is IReadOnlyDisabledFormComponent readOnlyDisabledFormComponent)
{
readOnlyDisabledFormComponent.Disabled = _disabled;
readOnlyDisabledFormComponent.StateHasChanged();
}
}Since you check in where clause is IReadOnlyDisabledFormComponent and then still use a direct cast, but up to you tbh, a really minor readability upgrade in exchange for more lines
|
The problem with my current interface approach is the user set I could do the same here but it would result in redundant code in every class that currently implements The other alternative is to use @dennisrahmen's solution instead and provide no opportunity for overriding the Form value from the child (unless it is null, in which case it is ignored). What are your thoughts @henon @ScarletKuro? |
|
An example of the code required in a child component: internal bool IsReadOnly => ReadOnly || (Form != null && Form is MudForm mudForm && mudForm.ReadOnly);
internal bool IsDisabled => Disabled || (Form != null && Form is MudForm mudForm && mudForm.Disabled);This is without adding the |
If I understand correctly @dennisrahmen and @ScarletKuro both suggested the same thing, Kuro specified it more exactly though. I agree that setting the form Disabled (or similarly Readonly) to disable or undisable all children regardless of their settings is probably the most useful use-case and what people would expect. Isn't this also consistent to how radio group works? |
|
@henon yes but I pointed out that there should be a 'null' option because when I set the parameter Disabled on the parent with a variable it would always be either true or false. Having it also null I could still use the individual components settings to set only one to Disable true. |
|
Absolutely, I agree. |
Yes. It just means that I can't set the respective property from the form and call it a day, as this would overwrite the child. I need code in each component like in my comment above that handles the required OR logic. It needs to check its own property and OR that with the property on the form, which doesn't work past the first render if the form overwrites the child property. The easy route is to use null as the default state and still override the child properties when the parent property is not null. This might be confusing if someone sets Disabled=false on the Form and later tries to set Disabled=true on the child - After a render, I believe it would apply the Form value again and ignore that the value was set to true on the child. |
|
I tried to make my own prototype without looking at MudBlazor implementation, so I'm not even sure it suits for MudBlazor. <MudForm IsDisabled="isFormDisabled">
<Component1/>
<Component2 IsDisabled="true" />
<Component3 IsDisabled="false" />
</MudForm>
If it's hard to read / understand here is the code to play with https://github.com/ScarletKuro/BlazorDisabled not really liking the solution since it violates BL0007, tho MudBlazor is overriding many [Parameter} auto-properties anyway. Yeah, sounds that null route is easier and yet I'm also not sure about this one:
UPD 3/25/2023! I slightly updated the code, and I also added alternative branch that uses |
|
@ScarletKuro Thanks so much for putting that example together! Sorry for not looking at it sooner, I didn't look over the weekend and wasn't working yesterday. I think the cascading parameter version is probably the best option. There is also no interface requirement in this version because the Form never interacts with the ReadOnly/Disabled parameters. This also means it won't violate BL0007, though we have been doing this elsewhere anyway. |
|
@henon That should do it for the updated PR. |
|
Seems like this was a lot of work, very much appreciated @Mr-Technician! |
* Add ReadOnly and Disabled properties to MudForm. * Rework readonly/disabled handling of all form components
|
A really useful and important PR. Thank you so much, @Mr-Technician! :-) |
* Add ReadOnly and Disabled properties to MudForm. * Rework readonly/disabled handling of all form components
Description
This PR adds ReadOnly and Disabled properties to the MudForm. They will automatically apply to all children forms and inputs to simplify working with large forms.
How Has This Been Tested?
I have added unit tests.
Types of changes
Checklist:
dev).