Skip to content

MudFileUpload: Use ParameterState#9264

Merged
ScarletKuro merged 3 commits intoMudBlazor:devfrom
ScarletKuro:fileupload_param
Jul 8, 2024
Merged

MudFileUpload: Use ParameterState#9264
ScarletKuro merged 3 commits intoMudBlazor:devfrom
ScarletKuro:fileupload_param

Conversation

@ScarletKuro
Copy link
Member

Description

The main problem is that MudFileUpload uses the MudFormComponent, which contains this pesky _value property that MudFileUpload uses as its main value.

The ParameterState doesn't have any mechanism to keep multiple backing fields in sync (and I don't think it should have any). Therefore, I came up with a proxy mechanism:

+protected virtual T? ReadValue()
+protected virtual Task WriteValueAsync(T? value)

This uses _value by default. However, if you want to use any other property as the main backing field for the MudFormComponent, you can override it. This is a good solution because it's not a breaking change, and you don't have to change other components that use MudFormComponent. With this, you can slowly move away from _value, which forces you to use logic in parameters.

How Has This Been Tested?

Manually and current unit tests.

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 bug Unexpected behavior or functionality not working as intended PR: needs review labels Jun 27, 2024
@ScarletKuro
Copy link
Member Author

@igotinfected in case you want to do some manual testing, since you have experience with this component. So far, I haven't found any problems.

@codecov
Copy link

codecov bot commented Jun 27, 2024

Codecov Report

Attention: Patch coverage is 93.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.98%. Comparing base (28bc599) to head (9eaf0c1).
Report is 299 commits behind head on dev.

Files Patch % Lines
...lazor/Components/FileUpload/MudFileUpload.razor.cs 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #9264      +/-   ##
==========================================
+ Coverage   89.82%   90.98%   +1.15%     
==========================================
  Files         412      403       -9     
  Lines       11878    12561     +683     
  Branches     2364     2438      +74     
==========================================
+ Hits        10670    11429     +759     
+ Misses        681      595      -86     
- Partials      527      537      +10     

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

@igotinfected
Copy link
Member

LGTM, didn't see anything out of the ordinary in the docs page. For the NotifyValueChangedAsync method, in my PR I have the form code (BeginValidate..., FieldChanged) in the ParameterState's ChangeHandler, does it matter you think? I'm assuming it doesn't make that big of a difference other than letting the ParameterState whether the value truly changed.

@ScarletKuro
Copy link
Member Author

does it matter you think? I'm assuming it doesn't make that big of a difference other than letting the ParameterState whether the value truly changed.

It doesn't matter.

Btw, shouldn't ClearAsync also call ResetValueAsync?
On https://dev.mudblazor.com/components/fileupload#drag-and-drop-example-with-form-validation, I noticed if you add a file and click "Clear", the IsTouched will stay true. Shouldn't it clear or is this expected? Sorry, I really have low knowledge of how form validation should perform in MudBlazor.

@igotinfected
Copy link
Member

At work we use IsTouched/Touched to know whether a user has interacted with the form.

If the user loads an edit page with data already pre-filled and they just want to remove an image from a file upload that is not Required (via ClearAsync), then the form/component should be marked as Touched to indicate user interaction, we use IsTouched to disable/enable the submit button forcing the user to make at least 1 change before being able to submit.

I did notice that in MudForm.razor.cs there is code that checks whether all Required components have been Touched. Probably implies that the forms we allow users to submit at work are considered invalid by MudBlazor:

var no_errors = _formControls.All(x => x.HasErrors == false);
var required_all_touched = _formControls.Where(x => x.Required).All(x => x.Touched);
var valid = no_errors && required_all_touched;

From my POV if Required components have a valid Value / pass validation, they should be Valid, regardless of whether they have been Touched or not.

So to answer your question, probably depends on what the consensus is for the purpose of IsTouched/Touched for the team.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Unexpected behavior or functionality not working as intended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants