Skip to content
Open
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -658,6 +658,11 @@
If true, the ClassValue property will not include the EditContext's FieldCssClass.
</summary>
</member>
<member name="P:Microsoft.FluentUI.AspNetCore.Components.FluentInputBase`1.ParsingErrorMessage">
<summary>
Gets or sets the error message to show when the field can not be parsed.
</summary>
</member>
<member name="P:Microsoft.FluentUI.AspNetCore.Components.FluentInputBase`1.EditContext">
<summary>
Gets the associated <see cref="T:Microsoft.AspNetCore.Components.Forms.EditContext"/>.
Expand All @@ -669,6 +674,12 @@
Gets the <see cref="P:Microsoft.FluentUI.AspNetCore.Components.FluentInputBase`1.FieldIdentifier"/> for the bound value.
</summary>
</member>
<member name="P:Microsoft.FluentUI.AspNetCore.Components.FluentInputBase`1.FieldDisplayName">
<summary>
Gets the display name of the field, using the specified display name if set; otherwise, uses the field
identifier's name if the field is bound.
</summary>
</member>
<member name="P:Microsoft.FluentUI.AspNetCore.Components.FluentInputBase`1.CurrentValue">
<summary>
Gets or sets the current value of the input.
Expand Down Expand Up @@ -8031,11 +8042,6 @@
An Id value must be set to use this property.
</summary>
</member>
<member name="P:Microsoft.FluentUI.AspNetCore.Components.FluentNumberField`1.ParsingErrorMessage">
<summary>
Gets or sets the error message to show when the field can not be parsed.
</summary>
</member>
<member name="P:Microsoft.FluentUI.AspNetCore.Components.FluentNumberField`1.ChildContent">
<summary>
Gets or sets the content to be rendered inside the component.
Expand All @@ -8047,6 +8053,11 @@
unless an explicit value for Min or Max is provided.
</summary>
</member>
<member name="P:Microsoft.FluentUI.AspNetCore.Components.FluentNumberField`1.ParsingErrorMessage">
<summary>
Gets or sets the error message to show when the field can not be parsed.
</summary>
</member>
<member name="M:Microsoft.FluentUI.AspNetCore.Components.FluentNumberField`1.FormatValueAsString(`0)">
<summary>
Formats the value as a string. Derived classes can override this to determine the formatting used for <c>CurrentValueAsString</c>.
Expand Down Expand Up @@ -8995,6 +9006,11 @@
Fires when hovered value changes. Value will be null if no rating item is hovered.
</summary>
</member>
<member name="P:Microsoft.FluentUI.AspNetCore.Components.FluentRating.ParsingErrorMessage">
<summary>
Gets or sets the error message to show when the field can not be parsed.
</summary>
</member>
<member name="P:Microsoft.FluentUI.AspNetCore.Components.FluentRating.GroupName">
<summary />
</member>
Expand Down
12 changes: 12 additions & 0 deletions src/Core/Components/Base/FluentInputBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,12 @@ public abstract partial class FluentInputBase<TValue> : FluentComponentBase, IDi
[Parameter]
public virtual bool Embedded { get; set; } = false;

/// <summary>
/// Gets or sets the error message to show when the field can not be parsed.
/// </summary>
[Parameter]
public virtual string ParsingErrorMessage { get; set; } = "The {0} field must have a valid format.";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😱 I don't think it's a good idea to add a parameter to InputBase. Because that means you have to manage it for all child components. There are apparently more than 15 of them.

Image

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every derived component already must deal with parsing errors via:

/// <summary>
/// Parses a string to create an instance of <typeparamref name="TValue"/>. Derived classes can override this to change how
/// <see cref="CurrentValueAsString"/> interprets incoming values.
/// </summary>
/// <param name="value">The string value to be parsed.</param>
/// <param name="result">An instance of <typeparamref name="TValue"/>.</param>
/// <param name="validationErrorMessage">If the value could not be parsed, provides a validation error message.</param>
/// <returns>True if the value could be parsed; otherwise false.</returns>
protected abstract bool TryParseValueFromString(string? value, [MaybeNullWhen(false)] out TValue result, [NotNullWhen(false)] out string? validationErrorMessage);

So the claim β€œyou now have to manage it for all child components” is slightly misleading. They already do, just in a fragmented way.

Meanwhile this parameter:

  • is optional
  • has a sane default
  • does not require overrides
  • does not introduce breaking changes

And before this, we got hardcoded text sitting in either FluentInputExtensions.TryParseSelectableValueFromString() .
Or in any derived component where we had the formatting actually implemented.

However, we could implement interfaces such as IParsableComponent and ICultureSensitiveComponent in relevant derived components and move the parsing logic there.
And move the whole parsing part to IParsableComponent
This way we can use simple checks in an extension method eg:

public static bool TryParseValueFromString<TInput, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] TValue>(
    this TInput input, string? value,
    [MaybeNullWhen(false)] out TValue result,
    [NotNullWhen(false)] out string? validationErrorMessage) where TInput : FluentInputBase<TValue>, IParsableComponent
{
    try
    {
        // We special-case bool values because BindConverter reserves bool conversion for conditional attributes.
        if ((typeof(TValue) == typeof(bool) && TryConvertToBool(value, out result)) ||
            (typeof(TValue) == typeof(bool?) && TryConvertToNullableBool(value, out result)))
        {
            validationErrorMessage = null;
            return true;
        }

        var culture =
            input is ICultureSensitiveComponent c
                ? c.Culture
                : CultureInfo.CurrentCulture;

        if (BindConverter.TryConvertTo<TValue>(value, culture, out var parsedValue))
        {
            result = parsedValue;
            validationErrorMessage = null;
            return true;
        }

        result = default;
        validationErrorMessage = string.Format(input.ParsingErrorMessage, input.FieldDisplayName);
        return false;
    }
    catch (InvalidOperationException ex)
    {
        throw new InvalidOperationException($"{input.GetType()} does not support the type '{typeof(TValue)}'.", ex);
    }
}

This approach would require additional work and testing. However, since this PR targets v4 and not the upcoming v5, which is where we’re planning bigger changes, I don’t see a strong benefit to implementing it right now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, We should not do that for v4. Having a sane default is enough, I think.


/// <summary>
/// Gets the associated <see cref="Microsoft.AspNetCore.Components.Forms.EditContext"/>.
/// This property is uninitialized if the input does not have a parent <see cref="EditForm"/>.
Expand All @@ -145,6 +151,12 @@ public abstract partial class FluentInputBase<TValue> : FluentComponentBase, IDi

internal virtual bool FieldBound => Field is not null || ValueExpression is not null || ValueChanged.HasDelegate;

/// <summary>
/// Gets the display name of the field, using the specified display name if set; otherwise, uses the field
/// identifier's name if the field is bound.
/// </summary>
internal string FieldDisplayName => DisplayName ?? (FieldBound ? FieldIdentifier.FieldName : UnknownBoundField);

protected async Task SetCurrentValueAsync(TValue? value)
{
var hasChanged = !EqualityComparer<TValue>.Default.Equals(value, Value);
Expand Down
6 changes: 3 additions & 3 deletions src/Core/Components/DateTime/FluentCalendar.razor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -293,9 +293,9 @@ private async Task PickerYearSelectAsync(DateTime? year)
/// <summary />
protected override bool TryParseValueFromString(string? value, out DateTime? result, [NotNullWhen(false)] out string? validationErrorMessage)
{
BindConverter.TryConvertTo(value, Culture, out result);
validationErrorMessage = null;
return true;
bool success = BindConverter.TryConvertTo(value, Culture, out result);
validationErrorMessage = success ? null : string.Format(ParsingErrorMessage, FieldDisplayName);
return success;
}

/// <summary />
Expand Down
7 changes: 3 additions & 4 deletions src/Core/Components/DateTime/FluentDatePicker.razor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -145,10 +145,9 @@ protected override bool TryParseValueFromString(string? value, out DateTime? res
value = new DateTime(year, 1, 1).ToString(Culture.DateTimeFormat.ShortDatePattern);
}

BindConverter.TryConvertTo(value, Culture, out result);

validationErrorMessage = null;
return true;
bool success = BindConverter.TryConvertTo(value, Culture, out result);
validationErrorMessage = success ? null : string.Format(ParsingErrorMessage, FieldDisplayName);
return success;
}

private string PlaceholderAccordingToView()
Expand Down
11 changes: 5 additions & 6 deletions src/Core/Components/NumberField/FluentNumberField.razor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -86,12 +86,6 @@ public partial class FluentNumberField<TValue> : FluentInputBase<TValue>, IAsync
[Parameter]
public string? AutoComplete { get; set; }

/// <summary>
/// Gets or sets the error message to show when the field can not be parsed.
/// </summary>
[Parameter]
public string ParsingErrorMessage { get; set; } = "The {0} field must be a (valid) number.";

/// <summary>
/// Gets or sets the content to be rendered inside the component.
/// </summary>
Expand All @@ -105,6 +99,11 @@ public partial class FluentNumberField<TValue> : FluentInputBase<TValue>, IAsync
[Parameter]
public bool UseTypeConstraints { get; set; }

/// <summary>
/// Gets or sets the error message to show when the field can not be parsed.
/// </summary>
public override string ParsingErrorMessage { get; set; } = "The {0} field must be a (valid) number.";

private static readonly string _stepAttributeValue = GetStepAttributeValue();

// If type constraints is true and min is null, set min to the minimum value of TValue.
Expand Down
8 changes: 6 additions & 2 deletions src/Core/Components/Rating/FluentRating.razor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,11 @@ public partial class FluentRating : FluentInputBase<int>
[Parameter]
public EventCallback<int?> OnHoverValueChanged { get; set; }

/// <summary>
/// Gets or sets the error message to show when the field can not be parsed.
/// </summary>
public override string ParsingErrorMessage { get; set; } = "The {0} field must be a number.";

/// <summary />
private string GroupName => Id ?? $"rating-{Id}";

Expand All @@ -90,8 +95,7 @@ protected override bool TryParseValueFromString(string? value, [MaybeNullWhen(fa
}
else
{
validationErrorMessage = string.Format(CultureInfo.InvariantCulture,
"The {0} field must be a number.",
validationErrorMessage = string.Format(ParsingErrorMessage,
FieldBound ? FieldIdentifier.FieldName : UnknownBoundField);
return false;
}
Expand Down
2 changes: 1 addition & 1 deletion src/Core/Extensions/FluentInputExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ internal static class FluentInputExtensions
}

result = default;
validationErrorMessage = $"The {input.DisplayName ?? (input.FieldBound ? input.FieldIdentifier.FieldName : input.UnknownBoundField)} field is not valid.";
validationErrorMessage = string.Format(input.ParsingErrorMessage, input.FieldDisplayName);
return false;
}
catch (InvalidOperationException ex)
Expand Down
39 changes: 39 additions & 0 deletions tests/Core/DateTime/FluentDatePickerTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// This file is licensed to you under the MIT License.
// ------------------------------------------------------------------------

using System.Diagnostics.CodeAnalysis;
using System.Globalization;
using Bunit;
using Microsoft.AspNetCore.Components;
Expand Down Expand Up @@ -292,4 +293,42 @@ public void FluentDatePicker_OnDoubleClickEventTriggers()
// Assert
Assert.Equal(expected, actual);
}

[Theory]
[InlineData("02-22-2000", "en-GB", "02/22/2000")]
[InlineData("02-22-2000", "nl-BE", null)]
[InlineData("22-12-2000", "nl-BE", "22/12/2000")]
[InlineData("02/22/2000", "en-GB", "02/22/2000")]
[InlineData("abc", "en-GB", null)]
[InlineData("02022000", "en-GB", null)]
public void FluentDatePicker_TryParseValueFromString(string? value, string? cultureName, string? expectedValue)
{
// Arrange
var picker = new TestDatePicker();
var culture = cultureName != null ? CultureInfo.GetCultureInfo(cultureName) : CultureInfo.InvariantCulture;

// Act
picker.Culture = culture;
var successfullParse = picker.CallTryParseValueFromString(value, out var resultDate, out var validationErrorMessage);

// Assert
if (successfullParse)
{
Assert.Equal(expectedValue, resultDate?.ToString(culture.DateTimeFormat.ShortDatePattern, culture));
}
else
{
Assert.Null(resultDate);
Assert.NotNull(validationErrorMessage);
}
}

// Temporary class to expose protected method
private class TestDatePicker : FluentDatePicker
{
public bool CallTryParseValueFromString(string? value, out System.DateTime? result, [NotNullWhen(false)] out string? validationErrorMessage)
{
return base.TryParseValueFromString(value, out result, out validationErrorMessage);
}
}
}
Loading