Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support custom validation class names #24835

Merged
merged 3 commits into from
Aug 25, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/Components/Forms/src/EditContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ public EditContext(object model)
// really don't, you can pass an empty object then ignore it. Ensuring it's nonnull
// simplifies things for all consumers of EditContext.
Model = model ?? throw new ArgumentNullException(nameof(model));
Properties = new EditContextProperties();
}

/// <summary>
Expand Down Expand Up @@ -62,6 +63,11 @@ public FieldIdentifier Field(string fieldName)
/// </summary>
public object Model { get; }

/// <summary>
/// Gets a collection of arbitrary properties associated with this instance.
/// </summary>
public EditContextProperties Properties { get; }

/// <summary>
/// Signals that the value for the specified field has changed.
/// </summary>
Expand Down
63 changes: 63 additions & 0 deletions src/Components/Forms/src/EditContextProperties.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;

namespace Microsoft.AspNetCore.Components.Forms
{
/// <summary>
/// Holds arbitrary key/value pairs associated with an <see cref="EditContext"/>.
/// This can be used to track additional metadata for application-specific purposes.
/// </summary>
public sealed class EditContextProperties
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to not use an IDictionary<object,object> on the EditContext class directly? Are we trying to 'hide' functionality from the Dictionary?

{
// We don't want to expose any way of enumerating the underlying dictionary, because that would
// prevent its usage to store private information. So we only expose an indexer and TryGetValue.
private Dictionary<object, object>? _contents;

/// <summary>
/// Gets or sets a value in the collection.
/// </summary>
/// <param name="key">The key under which the value is stored.</param>
/// <returns>The stored value.</returns>
public object this[object key]
Copy link
Contributor

Choose a reason for hiding this comment

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

How do you feel about making this a Set method? It has slightly less behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure what you mean by having less behavior. Can you ping me on IM when you're available to clarify?

Overall I don't really mind what the API looks like here since it's so obscure - any reasonable option is fine with me.

Copy link
Contributor

Choose a reason for hiding this comment

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

It does not seem like we need the getter at all. If you think it's necessary, we should update the KeyNotFoundException to use the same error message as the dictionary's exception

{
get => _contents is null ? throw new KeyNotFoundException() : _contents[key];
set
{
_contents ??= new Dictionary<object, object>();
pranavkm marked this conversation as resolved.
Show resolved Hide resolved
_contents[key] = value;
}
}

/// <summary>
/// Gets the value associated with the specified key, if any.
/// </summary>
/// <param name="key">The key under which the value is stored.</param>
/// <param name="value">The value, if present.</param>
/// <returns>True if the value was present, otherwise false.</returns>
public bool TryGetValue(object key, [NotNullWhen(true)] out object? value)
{
if (_contents is null)
{
value = default;
return false;
}
else
{
return _contents.TryGetValue(key, out value);
}
}

/// <summary>
/// Removes the specified entry from the collection.
/// </summary>
/// <param name="key">The key of the entry to be removed.</param>
/// <returns>True if the value was present, otherwise false.</returns>
public bool Remove(object key)
{
return _contents?.Remove(key) ?? false;
}
}
}
71 changes: 71 additions & 0 deletions src/Components/Forms/test/EditContextTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.Linq;
using Xunit;

Expand Down Expand Up @@ -252,6 +253,76 @@ public void LookingUpModel_ThatOverridesGetHashCodeAndEquals_Works()
Assert.True(editContext.IsModified(editContext.Field(nameof(EquatableModel.Property))));
}

[Fact]
public void Properties_CanRetrieveViaIndexer()
{
// Arrange
var editContext = new EditContext(new object());
var key1 = new object();
var key2 = new object();
var key3 = new object();
var value1 = new object();
var value2 = new object();

// Initially, the values are not present
Assert.Throws<KeyNotFoundException>(() => editContext.Properties[key1]);

// Can store and retrieve values
editContext.Properties[key1] = value1;
editContext.Properties[key2] = value2;
Assert.Same(value1, editContext.Properties[key1]);
Assert.Same(value2, editContext.Properties[key2]);

// Unrelated keys are still not found
Assert.Throws<KeyNotFoundException>(() => editContext.Properties[key3]);
}

[Fact]
public void Properties_CanRetrieveViaTryGetValue()
{
// Arrange
var editContext = new EditContext(new object());
var key1 = new object();
var key2 = new object();
var key3 = new object();
var value1 = new object();
var value2 = new object();

// Initially, the values are not present
Assert.False(editContext.Properties.TryGetValue(key1, out _));

// Can store and retrieve values
editContext.Properties[key1] = value1;
editContext.Properties[key2] = value2;
Assert.True(editContext.Properties.TryGetValue(key1, out var retrievedValue1));
Assert.True(editContext.Properties.TryGetValue(key2, out var retrievedValue2));
Assert.Same(value1, retrievedValue1);
Assert.Same(value2, retrievedValue2);

// Unrelated keys are still not found
Assert.False(editContext.Properties.TryGetValue(key3, out _));
}

[Fact]
public void Properties_CanRemove()
{
// Arrange
var editContext = new EditContext(new object());
var key = new object();
var value = new object();
editContext.Properties[key] = value;

// Act
var resultForExistingKey = editContext.Properties.Remove(key);
var resultForNonExistingKey = editContext.Properties.Remove(new object());

// Assert
Assert.True(resultForExistingKey);
Assert.False(resultForNonExistingKey);
Assert.False(editContext.Properties.TryGetValue(key, out _));
Assert.Throws<KeyNotFoundException>(() => editContext.Properties[key]);
}

class EquatableModel : IEquatable<EquatableModel>
{
public string Property { get; set; } = "";
Expand Down
32 changes: 22 additions & 10 deletions src/Components/Web/src/Forms/EditContextFieldClassExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Linq;
using System.Linq.Expressions;

namespace Microsoft.AspNetCore.Components.Forms
Expand All @@ -13,6 +12,8 @@ namespace Microsoft.AspNetCore.Components.Forms
/// </summary>
public static class EditContextFieldClassExtensions
{
private readonly static object FieldCssClassProviderKey = new object();

/// <summary>
/// Gets a string that indicates the status of the specified field as a CSS class. This will include
/// some combination of "modified", "valid", or "invalid", depending on the status of the field.
Expand All @@ -24,23 +25,34 @@ public static string FieldCssClass<TField>(this EditContext editContext, Express
=> FieldCssClass(editContext, FieldIdentifier.Create(accessor));

/// <summary>
/// Gets a string that indicates the status of the specified field as a CSS class. This will include
/// some combination of "modified", "valid", or "invalid", depending on the status of the field.
/// Gets a string that indicates the status of the specified field as a CSS class.
/// </summary>
/// <param name="editContext">The <see cref="EditContext"/>.</param>
/// <param name="fieldIdentifier">An identifier for the field.</param>
/// <returns>A string that indicates the status of the field.</returns>
public static string FieldCssClass(this EditContext editContext, in FieldIdentifier fieldIdentifier)
{
var isValid = !editContext.GetValidationMessages(fieldIdentifier).Any();
if (editContext.IsModified(fieldIdentifier))
{
return isValid ? "modified valid" : "modified invalid";
}
else
var provider = editContext.Properties.TryGetValue(FieldCssClassProviderKey, out var customProvider)
? (FieldCssClassProvider)customProvider
: FieldCssClassProvider.Instance;

return provider.GetFieldCssClass(editContext, fieldIdentifier);
}

/// <summary>
/// Associates the supplied <see cref="FieldCssClassProvider"/> with the supplied <see cref="EditContext"/>.
/// This customizes the field CSS class names used within the <see cref="EditContext"/>.
/// </summary>
/// <param name="editContext">The <see cref="EditContext"/>.</param>
/// <param name="fieldCssClassProvider">The <see cref="FieldCssClassProvider"/>.</param>
public static void SetFieldCssClassProvider(this EditContext editContext, FieldCssClassProvider fieldCssClassProvider)
{
if (fieldCssClassProvider is null)
{
return isValid ? "valid" : "invalid";
throw new ArgumentNullException(nameof(fieldCssClassProvider));
}

editContext.Properties[FieldCssClassProviderKey] = fieldCssClassProvider;
}
}
}
35 changes: 35 additions & 0 deletions src/Components/Web/src/Forms/FieldCssClassProvider.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.Linq;

namespace Microsoft.AspNetCore.Components.Forms
{
/// <summary>
/// Supplies CSS class names for form fields to represent their validation state or other
/// state information from an <see cref="EditContext"/>.
/// </summary>
public class FieldCssClassProvider
Copy link
Contributor

Choose a reason for hiding this comment

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

How do you feel about this being an abstract base type here? It makes it very clear to the user what APIs they would need to implement (in this case just one) to get the correct behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you propose we also have a public DefaultFieldCssClassProvider so that people implementing a custom one still have a way of using the default behaviors in some cases?

I did consider that before but TBH it felt no better for usability. We're then relying on people discovering the DefaultFieldCssClassProvider.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not a great deal of code to copy if they need the framework defaults. But if you think it's valuable, we can let this be.

{
internal readonly static FieldCssClassProvider Instance = new FieldCssClassProvider();

/// <summary>
/// Gets a string that indicates the status of the specified field as a CSS class.
/// </summary>
/// <param name="editContext">The <see cref="EditContext"/>.</param>
/// <param name="fieldIdentifier">The <see cref="FieldIdentifier"/>.</param>
/// <returns>A CSS class name string.</returns>
public virtual string GetFieldCssClass(EditContext editContext, in FieldIdentifier fieldIdentifier)
{
var isValid = !editContext.GetValidationMessages(fieldIdentifier).Any();
if (editContext.IsModified(fieldIdentifier))
{
return isValid ? "modified valid" : "modified invalid";
}
else
{
return isValid ? "valid" : "invalid";
}
Comment on lines +24 to +32
Copy link
Member

@javiercn javiercn Aug 25, 2020

Choose a reason for hiding this comment

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

nit: Extra pranav points :)

Suggested change
var isValid = !editContext.GetValidationMessages(fieldIdentifier).Any();
if (editContext.IsModified(fieldIdentifier))
{
return isValid ? "modified valid" : "modified invalid";
}
else
{
return isValid ? "valid" : "invalid";
}
var isValid = !editContext.GetValidationMessages(fieldIdentifier).Any();
var isModified = editContext.IsModified(fieldIdentifier);
return (isModified, isValid) switch {
(true, true) => "modified valid",
(true, true) => "modified invalid",
(false, true) => "valid",
(false, false) => "invalid"
};

}
}
}
17 changes: 17 additions & 0 deletions src/Components/test/E2ETest/Tests/FormsTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -560,6 +560,23 @@ public void SelectComponentSupportsOptionsComponent()
Browser.Equal("", () => selectWithoutComponent.GetAttribute("value"));
}

[Fact]
public void RespectsCustomFieldCssClassProvider()
{
var appElement = MountTypicalValidationComponent();
var socksInput = appElement.FindElement(By.ClassName("socks")).FindElement(By.TagName("input"));
var messagesAccessor = CreateValidationMessagesAccessor(appElement);

// Validates on edit
Browser.Equal("valid-socks", () => socksInput.GetAttribute("class"));
socksInput.SendKeys("Purple\t");
Browser.Equal("modified valid-socks", () => socksInput.GetAttribute("class"));

// Can become invalid
socksInput.SendKeys(" with yellow spots\t");
Browser.Equal("modified invalid-socks", () => socksInput.GetAttribute("class"));
}

[Fact]
public void NavigateOnSubmitWorks()
{
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Linq;
using Microsoft.AspNetCore.Components.Forms;

namespace BasicTestApp.FormsTest
{
// For E2E testing, this is a rough example of a field CSS class provider that looks for
// a custom attribute defining CSS class names. It isn't very efficient (it does reflection
// and allocates on every invocation) but is sufficient for testing purposes.
public class CustomFieldCssClassProvider : FieldCssClassProvider
{
public override string GetFieldCssClass(EditContext editContext, in FieldIdentifier fieldIdentifier)
{
var cssClassName = base.GetFieldCssClass(editContext, fieldIdentifier);

// If we can find a [CustomValidationClassName], use it
var propertyInfo = fieldIdentifier.Model.GetType().GetProperty(fieldIdentifier.FieldName);
if (propertyInfo != null)
{
var customValidationClassName = (CustomValidationClassNameAttribute)propertyInfo
.GetCustomAttributes(typeof(CustomValidationClassNameAttribute), true)
.FirstOrDefault();
if (customValidationClassName != null)
{
cssClassName = string.Join(' ', cssClassName.Split(' ').Select(token => token switch
{
"valid" => customValidationClassName.Valid ?? token,
"invalid" => customValidationClassName.Invalid ?? token,
_ => token,
}));
}
}

return cssClassName;
}
}

[AttributeUsage(AttributeTargets.Property)]
public class CustomValidationClassNameAttribute : Attribute
{
public string Valid { get; set; }
public string Invalid { get; set; }
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@
</InputRadioGroup>
</InputRadioGroup>
</p>
<p class="socks">
Socks color: <InputText @bind-Value="person.SocksColor" />
</p>
<p class="accepts-terms">
Accepts terms: <InputCheckbox @bind-Value="person.AcceptsTerms" title="You have to check this" />
</p>
Expand Down Expand Up @@ -98,6 +101,7 @@
protected override void OnInitialized()
{
editContext = new EditContext(person);
editContext.SetFieldCssClassProvider(new CustomFieldCssClassProvider());
customValidationMessageStore = new ValidationMessageStore(editContext);
}

Expand Down Expand Up @@ -145,6 +149,9 @@
[Required, EnumDataType(typeof(Country))]
public Country? Country { get; set; } = null;

[Required, StringLength(10), CustomValidationClassName(Valid = "valid-socks", Invalid = "invalid-socks")]
public string SocksColor { get; set; }

public string Username { get; set; }
}

Expand Down