Add CssClass property to ValidationSummary

[ChristopherHaws]

Add CssClass property to ValidationSummary

API Proposal: #46461

Description

Add CssClass property to ValidationSummary and append the contents to the resulting class attribute.

Fixes #43860, Closes: #46461

[ghost]

Thanks for your PR, @ChristopherHaws. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

[ChristopherHaws]

@SteveSandersonMS I updated the test to be an E2E test. I had a hell of a time getting selenium to work because I have v105 of chrome installed but the web driver used by the E2E tests is still v103 and chrome doesn't supply old versions of chrome. I had to lookup the build number of chromium that corresponds to v103 of chrome and use that instead. I remember now why I stopped using selenium and switched to playwright. :)

I think this PR is ready for review now. Thanks!

[mkArtakMSFT]

Thanks for your PR, @ChristopherHaws.
@SteveSandersonMS could you please review this? Thanks!

[ghost]

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no breaking changes are introduced, please remove the pending ci rerun label to kick off a new CI run.

[mkArtakMSFT]

@Nick-Stanton reassigning this to you as @SteveSandersonMS has been busy with other things.

[Nick-Stanton]

Hi @ChristopherHaws, thank you for your contribution and apologies for the delay. I made one small request, but otherwise the changes look good.

Since your PR contains new API, you will need to create an API Proposal to go along with your issue. This is done by opening a new issue using the API Proposal template. More information about the process is here. Once the proposal is ready, I will "champion" your changes on your behalf during review. Please let me know if you have any questions.

[ghost]

/azp run

[azure-pipelines]

Commenter does not have sufficient privileges for PR 43928 in repo dotnet/aspnetcore

[Nick-Stanton]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[ghost]

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no breaking changes are introduced, please remove the pending ci rerun label to kick off a new CI run.

[ghost]

/azp run

[azure-pipelines]

Commenter does not have sufficient privileges for PR 43928 in repo dotnet/aspnetcore

[Nick-Stanton]

@ChristopherHaws thanks for taking the time to draft your proposal. It looks like the commit that moved your new API caused some new build failures. I imagine CI will be clean once those are fixed.

While investigating these changes deeper to prepare for API review, I had a couple questions:

1. First, I was looking at your example in Pass css class in <ValidationSummary /> to the generated <ul> #43860. If you were to pass class="validation-errors pt-2", wouldn't you get your desired output? I ask because if that scenario works and this feature hasn't been asked for elsewhere, it may be hard to argue for this change. Example:

<ValidationSummary class="validation-errors pt-2" data-test="validation-errors" />

2. This implementation has ValidationSummary modifying its own CssClass parameter, which you were right to ask about here Pass css class in <ValidationSummary /> to the generated <ul> #43860 (comment). We usually guide against doing this. Is there a way to achieve this functionality without directly changing CssClass?

[ChristopherHaws]

@Nick-Stanton Hmm, not sure why the build would be failing. Unfortunately I don't have time to look at that this week, I am super busy at work and have a theater show I am running at night.

1. The existing <ValidationSummary /> component adds validation-errors, not the end user.
2. If I remember correctly (it's been almost 6 months since I wrote this code), the CssClass property needs to contain the class names being appended to the component which includes the built-in validation-errors css class.

The only built-in component I found which allows users to set CssClass is NavLink) which also appends it's built-in css class to the property.

Most of the built-in components don't allow you to pass CssClass (it is get only).

I don't know the exact reason that the built-in css class names need to be in this property but I am guessing it is one or many of the following:

• Performance. If you append the string only one time when the parameter is set, you avoid string allocations every time the component gets rendered/re-rendered
• If someone has a ref to the component, they could then get the CssClass and it would actually contain all of the css classes, not just the ones the user added
• When writing tests, this would allow you to assert that the css classes are correct without needing to validate it using the DOM

[SteveSandersonMS]

The only built-in component I found which allows users to set CssClass is NavLink) which also appends it's built-in css class to the property.

Ah, checking this more closely, I see that the CssClass property you're adding is not actually a [Parameter] property so is fine to mutate. (Apologies to @Nick-Stanton - I gave an incorrect suggestion about this.). However I'm not sure it should be settable by subclasses, since their changes would be overwritten every time OnParametersSet runs which would be very confusing.

The existing component adds validation-errors, not the end user.

That's right, validation-errors is the default CSS class attribute value. However it's intentional that developers can overwrite this with any other that they want. For example, maybe their styling system doesn't use that CSS class name, so they instead want to use <ValidationErrors class="my-errors">. It's intended as a feature that you can replace the class name. It's the same with ValidationMessage too.

The change in the PR, if I'm reading correctly, would prevent that and force people to have validation-errors whether or not they want it. I know in most cases that won't cause them any problem, but I'm not really sold on the idea that a lot of people are going to be using class to represent additional class names as opposed to thinking they are setting the entire class attribute. Especially given how this would then be inconsistent with ValidationMessage.

The reason why we combine classes dynamically in NavLink is that NavLink adds and removes the active class based on navigation state. There has to be some dynamic adding/removing of this class since that's the whole point of NavLink. However we still don't force any specific hardcoded class name since you can set ActiveClass to an arbitrary value.

In summary it looks as if we already have the desired functionality, i.e., the ability to customize the CSS class name and add arbitrary extra ones just by specifying class. I don't think we'd want to force people to have validation-errors. Hope that seems reasonable. If I'm missing something important, please let me know!

[ghost]

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no breaking changes are introduced, please remove the pending ci rerun label to kick off a new CI run.

[ChristopherHaws]

@SteveSandersonMS The current behavior of ValidationSummary is that you simply cannot override the class attribute at all. If you pass a class, it is simply ignored. Here are two examples I just tested:

<!-- <ValidationSummary /> -->
<ul class="validation-errors">
    <li class="validation-message">...</li>
</ul>

<!-- <ValidationSummary class="test" data-test="test" /> -->
<ul data-test="test" class="validation-errors">
    <li class="validation-message">...</li>
</ul>

I see your point about it being confusing as to whether it replaces or appends the class though. I am coming to blazor after using React for many years where it is pretty common for the classes you pass in to be appended, so I would have assumed that it would be appended. Here are the docs for FluentUI React where you can see that className is an Optional class name that is added to the container of the component.

I get that this is not React, so I looked at one of the top Blazor Component libraries to see what they do. MudBlazor defines Class as User class names, separated by space. - The key word here being that this is the "User"'s class names.

[SteveSandersonMS]

@ChristopherHaws Can you provide a repro using an online REPL?

I just tried it and it works for me: https://blazorrepl.telerik.com/QRumvqaN29a37x4K49

[ChristopherHaws]

Interesting, I had used BlazorFiddle to try it. I guess they are on an old version of Blazor. It's been a while since I was dealing with this issue in our project (we ended up just wrapping ValidationSummary in our own component).

[Nick-Stanton]

@ChristopherHaws I just looked into it and it looks like they're using a really old version of Blazor.

Apologies for not discovering the functionality earlier in review. Do you still have interest in pursuing this PR? It seems like the ask is already met.

[mkArtakMSFT]

Closing as this is already supported.