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

Added Blazor EditForm input validation support for some components #1141

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

gorogdemi
Copy link

Pull Request

🤨 Rationale

Currently there is no proper EditForm support for Nimble input components in Blazor, therefore the ErrorVisible and ErrorText attributes do not get automatically changed (like with the standard input components Blazor provides).

This PR adds support for showing the input validation errors automatically for the following input components:

  • NimbleInputText
  • NimbleInputNumber
  • NimbleSelect
  • NimbleCombobox

Partially covers #766

👩‍💻 Implementation

Basically we want to change the ErrorVisible and ErrorText properties at the same time, so the expected behavior is when the ErrorText is not empty then we want ErrorVisible to be true and vice versa.

Many components are inherited from the NimbleInputBase class, but most of them do not have/support the ErrorVisible/Text attributes/properties, so I chose not to move these properties from each component to the base class in order to use them.

Instead, I changed the UpdateAdditionalValidationAttributes in NimbleInputBase to add and remove the error-text and error-visible attributes to/from the AdditionalAttributes dictionary.
This implementation does not affect the behavior when the Nimble input components are not part of an EditForm, so they still can be set manually in that case.

I also updated the summary section of each supported component.

🧪 Testing

I tested the behavior in my sample project manually and it works as expected.
I'm not sure if automated tests are needed and if so how to do it well, since there is no tests for the NimbleInputBase yet anyway.

✅ Checklist

  • I have updated the project documentation to reflect my changes or determined no changes are needed.

additionalAttributes["aria-invalid"] = true;
additionalAttributes["error-visible"] = true;
additionalAttributes["error-text"] = validationMessages.First();
Copy link
Author

Choose a reason for hiding this comment

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

Note: we show only the first validation message, since the Nimble components' design enables only one to be shown nicely.

Copy link
Contributor

Choose a reason for hiding this comment

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

To begin with, we ultimately would like to move away from the AdditionalAttributes pattern to promote stronger typing of known attributes that we want clearly exposed on our APIs (though this could be a problematic goal, given how extensively this could break people).

Additionally, introducing knowledge of the error-visible and error-text attributes into this class seems like a subversion of the existing APIs that are present (i.e. NimbleInputBase does not own the ErrorText or ErrorVisible properties). It's possible that this is potentially how the API should evolve, where NimbleInputBase does indeed expose those properties, but I haven't fully wrapped my head around the
implications/benefits/drawbacks of this yet.

At the moment, I don't have a strong sense for the right way to provide the capabilities you are requesting. The approach that I'm toying with in my head is something like the following:

  • Create a new intermediate abstract Component class ErrorInput, which hosts the ErrorText and ErrorVisible properties.
    • Additionally, this class would have public read-only properties (with protected setters) ActualErrorInput and ActualErrorVisible. Updates to ErrorText and ErrorVisible would update their Actual<> counterparts. The reasoning behind a pattern like this is to prevent stomping on the user-settable properties of the components. The various concrete ErrorInput components (like NimbleTextField) would then bind their attributes to the Actual<> properties.
  • Make OnValidateStateChanged on NimbleInputBase protected virtual.
  • Override OnValidteStateChanged on ErrorInput and set the Actual<> properties as needed with the validation information available on the EditContent.

I'm also unsure if we should be handling the aria-invalid attribute at this level, but I think that is ultimately orthogonal to the capabilities we're trying to add here.

@rajsite and @msmithNI please chime in with your own thoughts as well.

Copy link
Member

@rajsite rajsite Mar 31, 2023

Choose a reason for hiding this comment

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

I think my concerns are:

  • +1 that we want to move away from AdditionalAttributes to provide stronger typing

  • It's not clear if the user intentionally specifies ErrorText and ErrorVisible how that interacts with the validation results. I think this is testable in the current infrastructure, i.e. testing validation behavior and emitted attributes.

  • My understanding is that Blazor lets you opt-in to visualizing validation state. You have to manually add ValidationMessage or ValidationSummary components to do the visualization.

    We define the visual for validation sure (via error-text and error-visible), but I think we should have a way to opt-in or opt-out of the presentation for a control in an edit context. We have talked about it being frustrating that just because we are in an edit context we are forced to define a value expression. We shouldn't also force visualizing the validation result without a way to easily opt-out.

Maybe we say that by default most users want the automatic validation and make it opt-out?

<NimbleTextField CustomError="True" @bind-Value="exampleModel.MyValue" ErrorText="My custom message" @bind-ErrorVisible="exampleModel.ErrorVisible">

Or to make it opt-in which is more like Blazor's ValidationMessage pattern, let you select the validation context. For example, implement something like the ValidationMessage's For property:

<NimbleTextField @bind-Value="exampleModel.MyValue" ErrorTextFor="@(() => exampleModel.MyValue)">

That gives you a lot of control, the error state of the text field doesn't have to correspond only to the text field value, but maybe that is not a useful degree of freedom.

P.S. / Aside

though this could be a problematic goal, given how extensively this could break people

We could mitigate the amount it breaks people. The property names could match the common in-use additional attribute names, i.e. class instead of Class. Maybe we can make them alias each other, i.e. class is just an accessor for Class, and mark the lowercase version deprecated.

Copy link
Member

Choose a reason for hiding this comment

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

@gorogdemi we want to come back around to this discussion but haven't had a chance to investigate further yet. Do you have a reasonable alternative workaround until then?

Copy link
Author

Choose a reason for hiding this comment

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

I will really need this in H2, but wanted to go ahead and make sure this will be completed until then. I can still use the ValidationMessage component until this gets completed.

@rajsite
Copy link
Member

rajsite commented Apr 20, 2023

Moving to draft until we can pick this back up

@rajsite rajsite marked this pull request as draft April 20, 2023 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants