microsoft / fluentui-blazor

Microsoft Fluent UI Blazor components library. For use with ASP.NET Core Blazor applications
https://www.fluentui-blazor.net
MIT License
3.89k stars 377 forks source link

fix: Missing support of unsigned numerics in `FluentNumberField` #2882

Open Eagle3386 opened 4 weeks ago

Eagle3386 commented 4 weeks ago

🐛 Bug Report

I chose "bug" over "feature request" since a component named FluentNumberField should have supported the unsigned equivalents of int, long, sbyte & short right from the start. Because I can't imagine a reason to not enable developers to limit sliders, steppers, etc. to positive values only (talking mostly age, RGB color & range pickers here, but applies to customer ID fields as well).

On a more personal note, I'm really having a hard time understanding why (C#) developers still prefer int CustomerId over uint CustomerId since such a property never really contains a negative integer. But I assume it's just like with decimal which - besides scientific calculations - should always be preferred over float & double (AFAIK, even the C# docs explicitly state this).. 😅

💻 Repro or Code Sample

@* // … code left out for brevity… *@
<FluentNumberField HideStep="true" Immediate="true" Label="Customer #:" Max="999999" MaxLength="6"
                   Min="100000" MinLength="6" Required="true" @bind-Value="Account.CustomerId" />
@* // … code left out for brevity… *@
@code
{
  private AccountData Account { get; } = new();
  // … code left out for brevity…
  public record AccountData
  {
    internal uint? CustomerId { get; set; }
    // … code left out for brevity…
  }
}

🤔 Expected Behavior

As I explicitly declared HideStep="true" above, the following method shouldn't even get called. However, if it's necessary for some reason, this code inside FluentNumberField.razor.cs:

private static string GetStepAttributeValue()
{
    // Unwrap Nullable<T>, because InputBase already deals with the Nullable aspect
    // of it for us. We will only get asked to parse the T for nonempty inputs.
    var targetType = Nullable.GetUnderlyingType(typeof(TValue)) ?? typeof(TValue);
    if (targetType == typeof(sbyte) ||
        targetType == typeof(int) || typeof(by)
        targetType == typeof(long) ||
        targetType == typeof(short) ||
        targetType == typeof(float) ||
        targetType == typeof(double) ||
        targetType == typeof(decimal))
    {
        return "1";
    }
    else
    {
        throw new InvalidOperationException($"The type '{targetType}' is not a supported numeric type.");
    }
}

should include the 4 unsigned numerics, too.

😯 Current Behavior

A InvalidOperationException is thrown:

System.InvalidOperationException: The type 'System.UInt32' is not a supported numeric type.
   at FluentNumberField`1[UInt32?].GetStepAttributeValue() in /_/src/Core/Components/NumberField/FluentNumberField.razor.cs:line 116
   at FluentNumberField`1[UInt32?]..cctor() in /_/src/Core/Components/NumberField/FluentNumberField.razor.cs:line 97

💁 Possible Solution

I'd like to do a PR to add those 4 types, if the maintainers would agree with me. 😉

🔦 Context

Provide my app's users with a couple of numeric input fields which are completely preventing any negative value.

On a side note, the XML Doc for Max & Min should clarify if Maxlength & Minlength are needed anyway or if the former already take care of that.

🌍 Your Environment

vnbaaij commented 4 weeks ago

I'd like to do a PR to add those 4 types, if the maintainers would agree with me. 😉

Please go for it! Just as a heads up, I did an initial quick try a while back and quickly ran into some issues...Don't say you were not warned!! (😂)

davhdavh commented 1 week ago

https://learn.microsoft.com/en-us/dotnet/api/system.numerics.iadditiveidentity-2?view=net-8.0

Isn't it more appropriate to use the defined interface that tells us if the type has the concept of a step rather than hardcoding the list?