Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9cd0161) 85.49% compared to head (3ce570d) 87.17%.

@@            Coverage Diff             @@
##              dev    #7720      +/-   ##
==========================================
+ Coverage   85.49%   87.17%   +1.68%     
==========================================
  Files         427      389      -38     
  Lines       15267    11411    -3856     
  Branches     3329     2297    -1032     
==========================================
- Hits        13052     9948    -3104     
+ Misses       1536      966     -570     
+ Partials      679      497     -182     

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

Adding @Mr-Technician, but I'm not really liking that we are adding an async call here in components parameter.
I know we have such pattern in MudBlazor's codebase, but we are trying to get rid of it and I would like to avoid it in the new code(this can lead to infinity render issue dotnet/aspnetcore#28518 especially that your property set is calling an FilesChanged depending on the codepath).
But I will let @Mr-Technician to decide it.

@ScarletKuro I was looking at this last night and had the same thought. I don't want to add an async call in the setter.

@igotinfected How about implementing a Clear button? This is how other Form components do it.

@Mr-Technician @ScarletKuro Fully agree with both of you guys, this was the quick and dirty version :)

I think for a clean Clear button approach we would have to make a breaking change on ButtonTemplate to provide a Clear callback via the context, similar to how Blazorise does it:

<ButtonsTemplate>
    <Progress Value="@filePickerCustom.GetProgressPercentage()" />
    <Buttons>
        <Button Clicked="@context.Clear" Color="Color.Warning"><Icon Name="IconName.Clear" /></Button>
        <Button Clicked="@context.Upload" Color="Color.Primary"><Icon Name="IconName.FileUpload" /></Button>
    </Buttons>
</ButtonsTemplate>

We would then have something akin to:

public RenderFragment<FileUploadButtonTemplateContext> ButtonTemplate { get; set; }

Where FileUploadButtonTemplateContext would be:

public class FileUploadButtonTemplateContext
{
    public string Id { get; set; }
    public EventCallback ClearAsync { get; set; }
}

Or potentially, based on the existing MudBlazor Context-type classes for the DataGrid component, something like:

public class FileUploadButtonTemplateContext<T>
{
    public string Id { get; }
    public FileUploadButtonTemplateActions Actions { get; }
    
    private readonly MudFileUpload<T> _fileUpload;
    
    public FileUploadButtonTemplateContext(MudFileUpload<T> fileUpload, string id)
    {
        _fileUpload = fileUpload;
        Id = id;
        Actions = new FileUploadButtonTemplateActions
        {
            ClearAsync = async () => await _fileUpload.ClearAsync(),
        };
    }

    public class FileUploadButtonTemplateActions
    {
        public Func<Task> ClearAsync { get; init; } = null!;
    }
}

Thoughts?

I was thinking we would add a ClearAsync method to the file upload component and not modify the context. It could still be cleared with a button using a reference to the component. This is potentially less clean but isn't breaking.

Having looked at this more closely on my computer, we might be able to get around the breaking change by add a ToString definition in the context class that returns the existing for id. This would allow most, if not all, existing code to still function as it does now.

I otherwise agree that using ClearAsync from a component reference is less clean. I would consider the FileUploadButtonTemplateContext to be the alternative to making FileUpload Clearable which I don't think we can do.

@Mr-Technician Thank you for the feedback! The .ToString() override is a brilliant solution, didn't think of that!

I've gone ahead and implemented the ClearAsync function, as well as the FileUploadButtonTemplateContext approach.

After adapting the snippet in the original post to use the new context, this is the result:

Working as expected :)

For regressions, there is already a test verifying the old context behaviour:

https://github.com/MudBlazor/MudBlazor/blob/9cd01611e0d8185cfa6c0f34e421874f5195e066/src/MudBlazor.UnitTests/Components/FileUploadTests.cs#L108C1-L122C10

I'm also fine with going for the ClearAsync solution first and moving the rest to 7.0 if you're more comfortable with that. Will add more tests targeting the new context class and a doc example if we go for it.

lgtm pending the new tests and docs example.

@ScarletKuro @henon Are both of you ok with merging this instead of waiting for v7? It's technically breaking but existing code will still work unless someone is doing something unusual. "Normal" usage of the button context will still work the same as it did before without modifying code due to the ToString method.

Updated all ButtonTemplate usages to use @context.Id instead of @context, minus the backwards compatibility test of course.

Added a render test, and two ...Context.Actions.ClearAsync tests, one of which validates Form state.

Updated documentation and added a second drag and drop example with a form using the new ButtonTemplate context.

Let me know if you have different ideas for documentation, can also add a recording of the new example if you need :)

Are both of you ok with merging this instead of waiting for v7

Personally I'm ok with this.

I'll do a final review and (hopefully) merge tomorrow afternoon. 👍