MarimerLLC / csla

A home for your business logic in any .NET application.
https://cslanet.com
MIT License
1.25k stars 402 forks source link

Enhanced Blazor ValidationMessage component to handle CSLA's severities #1295

Closed TheCakeMonster closed 4 years ago

TheCakeMonster commented 5 years ago

The Blazor ValidationMessage component included in .NET Core 3.0 displays all validation messages with the same style; at the moment there is no support in Blazor for the concept of rules with different severities, as we have in CSLA.

We could create a new Blazor component that allows messages of different severities to be visually distinct - probably by decorating the HTML generated with different CSS class names. This will allow developers to separate what messages are as a result of errors that prevent the form being submitted, from those that are warnings or information that do not prevent submission. Class names should be assigned to the component via parameters, so that they can be assigned by developers to suit their own naming conventions.

I would see this component being a replacement for the built in component in Blazor applications that use CSLA, where developers choose to make use of different rule severities. The existing component could still be used, but only where there is no requirement to segregate rules by severity.

Some options to consider; input is welcome

1) Use of CSS class names seems like a standard mechanism for offering this separation, and I think they can even be used to offer some accessibility guidance. Are there any Aria hints that might be an additional enhancement, to aid with the delivery of better accessibility?

2) Should messages of all severities be displayed at the same time, or should warnings and information only display when there are no errors?

3) Should we expose delegate parameters to which developers can bind boolean values to define whether (or when) they wish each of the lesser severities to display? Such parameters would be optional, so that the enhanced functionality they provide does not result in clutter for simpler use cases. Such parameters add complexity to the component, and are likely to increase the support and maintenance cost of it through its life, so it might be best to avoid this.

4) An alternative I considered is delivering a component that can be stacked or used in groups, so that people were able to use a separate component for each severity. To me, this seems like it might be overly complex and lead to very verbose Razor syntax in an edit form.

5) A more advanced option might be to offer a templated component, so that developers have complete control over the HTML used to output messages of each severity. However, this is likely to be too complex - and too verbose in Razor syntax - for most use cases. At best, this might be a more advanced component, provided in addition to a more basic one that we intend for the majority of circumstances. Perhaps this might be an appropriate method of delivering a ValidationSummary component at a later date?

6) We could default CSS class names to use bootstrap defaults, so that they work automatically for newly created Blazor projects - the templates are all based on bootstrap at the moment. However, any defaults we provide have to survive the test of time - I cannot see us being able to change them in the future without it being a breaking change for people. The Blazor templates could be updated at any point in the future to use something other than bootstrap, and at that point, our defaults may look inappropriate. As a result, I think it is probably a better approach to avoid specifically targeting existing bootstrap class names, unless they are defacto standards.

rockfordlhotka commented 5 years ago

Right now in the BlazorExample sample app the info/warn/error severity messages are all displayed via helper methods on a viewmodel type, so the razor looks like this:

          <div>
            <input @bind-value="vm.Model.Name" @bind-value:event="oninput" /><br />
            <span style="background-color:red;color:white">@vm.GetErrorText(nameof(vm.Model.Name))</span>
            <span style="background-color:yellow;color:black">@vm.GetWarningText(nameof(vm.Model.Name))</span>
            <span style="background-color:blue;color:white">@vm.GetInformationText(nameof(vm.Model.Name))</span>
          </div>

I do think it would be really nice to abstract that somehow, and yet not reduce the flexibility available to a UI designer in terms of what's displayed how or when.

Which isn't to say that having some opinionated defaults are bad, because I like that! But I'd like to hold to the philosophy of the XAML UI support, which is mostly to provide the tools for a UI designer to do anything they can image.

For example, in WPF there's Csla.Xaml.PropertyStatus which is an opinionated and rather inflexible control that provides a display much like the old Windows Forms ErrorProvider.

But there's also the invisible PropertyInfo control that exposes all the metastate for a property in a way such that PropertyInfo can be part of what's called a control template - so a UI designer or developer can use PropertyInfo plus other controls to create reusable UI widgets such as an input box that displays metastate in a standard manner throughout the app.

That is what I think would be ideal in Blazor. My problem is that I haven't had time to dig into figuring out how (or if) one might create a reusable "rich input box" widget in Blazor. If I understood that, then it would be easier to figure out how (or if) something like PropertyInfo would fit into the picture.

rockfordlhotka commented 5 years ago

In other words, can that chunk of razor in my previous post be somehow wrapped into a reusable component/widget/control? And if so, what would that look like?

rockfordlhotka commented 5 years ago

I also wonder if the razor code should look like this instead of the above:

<div>
  <input @bind-value="vm.Model.Name" @bind-value:event="oninput" /><br />
  <span style="background-color:red;color:white">@vm.PropertyInfo(nameof(vm.Model.Name)).ErrorText</span>
  <span style="background-color:yellow;color:black">@vm.PropertyInfo(nameof(vm.Model.Name)).WarningText</span>
  <span style="background-color:blue;color:white">@vm.PropertyInfo(nameof(vm.Model.Name)).InformationText</span>
</div>

Basically have the viewmodel base class expose a single PropertyInfo method to get one type that exposes all the metastate, rather than a whole bunch of individual methods like I have now.

Keep in mind it isn't just info/warn/error text. It is also whether the property is valid, busy, can be read, can be edited, etc. There's a lot of metastate per property!

TheCakeMonster commented 5 years ago

Yep, I see what you are saying. It sounds like the component I have listed in this issue is of the opinionated type, for use in simple/common scenarios.

We definitely could wrap all of that markup up together, but so far I have been imagining replacing individual components - in other words, not including the input control at this stage. Whilst this is a less visionary approach, it allows people to use other components as they want, and mix and match CSLA with non-CSLA alternatives. Because of Blazor's strong support for it, I can see the third party component library being a significant part of the ecosystem, so I feel it might be sensible to go with/retain the existing separation for the simpler, opinionated versions - at least to start with, before building on that for the more advanced versions. For example, if someone wants to use a third party component vendor's date selector control, then we should support that as simply as possible. Having a CSLA version of the ValidationMessage control therefore seems to have merit in its own right.

I'm not a XAML developer, so PropertyInfo isn't something of which I already have knowledge. However, it is definitely on my radar to understand it better before starting development. The purpose of raising the issue was not to indicate that I'm ready to start, but to give the wider community plenty of time to give me their opinions, if anyone has them.

From what you've said of PropertyInfo, it sounds a lot like templated components in Blazor. We can definitely offer that. However, it feels like doing the simpler, drop in replacements still have value, and might be a stepping stone to the templated version.

I think we are probably agreeing - that there is a benefit in exploring both options - both a simple, drop in replacement for the existing components, and a more advanced facility for those who want to go beyond the basic scenarios and have full control over what UI is exposed to the user.

rockfordlhotka commented 5 years ago

I think we are agreeing - and I appreciate this conversation, as this is exactly how to flesh out ideas.

Having an opinionated and simple solution is a good idea. That's why the PropertyStatus control exists for WPF. It may not be useful for everyone, but for a quick UI that needs to show error and busy status info it is great (it even includes its own busy animation built-in).

Also having a non-visual, non-prescriptive component for use by UI designers to create whatever kind of UI they like is also a good idea. That's where the PropertyInfo control came from, is this desire.

Ultimately I would like to see Blazor equivalents to both of those concepts.

(and after sleeping on it, I really do think I prefer having Csla.Blazor.ViewModel expose a single PropertyInfo method vs tons of individual methods for each aspect of metastate. It just seems like a cleaner implementation and an easier API to understand)

TheCakeMonster commented 5 years ago

I, too, appreciate the interaction. Not only is it directly enhancing what I'm trying to deliver into the CSLA project, but I'm learning a lot too. I'm happy to continue for as long as you can spare the time!

Do you think that the return from the PropertyInfo method would be technology-agnostic? Do you think that the ViewModel class should do the work of constructing the return, or something else? I wondered if the ViewModel doing the work might be too high up the tech stack - although maybe that's not what you have in mind. If the return is not specific to Blazor, then would there be benefit in putting the data-gathering/object construction aspect of this into the core, so that it could be used by multiple UI implementations in the future?

I have to confess at this point that I have an ulterior motive for asking - we don't currently use ViewModels. We probably should - and it could be that this conversation will change my mind - but just at the moment we don't. It is entirely acceptable for you to start your mental questioning of my sanity at this point, but bear with me! I took to heart your post from 2014 where I felt you suggested that, in general, MVVM was less beneficial for people using CSLA than those who have not yet seen the light. I've always carried in my mind the idea that the job (or one of the jobs) of a CSLA object was to offer functionality to the UI in whatever way it needed. Whilst this does muddy the waters as to quite what the responsibility of the CSLA object was when considered solely from the standpoint of the single responsibility principle, it seems, generally, to have stood us in quite good stead.

I'm wondering whether we can deliver what we are talking about here whilst allowing the ViewModel to remain optional. I'm sure there are lots of people who would want to use ViewModels, and supporting that is entirely valid and beneficial. However, I like the idea that the on-ramp for CSLA remain as shallow as it can be; if people can't take on board the ViewModel concept at the same time as everything else, then that could be OK, if we're not tied to them. This could be possible whether you recommend them or not - that's a different thing.

I'm completely happy to be shot down on this. I know you have a lot more experience of delivering complex and rich client UIs than I do, as well as much more of the knowledge of the computing science theories behind the industry. What I'm doing is posing the idea that there remains some merit to offering a simple approach for people new to CSLA and Blazor, by making the more advanced concepts optional to begin with.

I was pondering of whether a valid approach might be to expose the PropertyInfo from the ViewModel through a convenience method, but that the work could be done by another class, lower down. That lower class could then be used by the UI components I am building, so that they work without the end user knowing that there is such a thing as a ViewModel, if they want. Indeed, I even wondered whether a convenience method like this could go into BusinessBase. I'm not suggesting that the code to gather/build the object should be in BusinessBase - here's an opportunity for composition - but that access to the work done by an external class is also exposed via a method in BusinessBase.

Is this too outdated a concept, from a maturing dog attempting to keep up with modern tricks? Should ViewModels be the way everyone works these days?

I look forward to your thoughts. Which, admittedly, might be slightly off-topic in relation to the original feature request!

TheCakeMonster commented 5 years ago

I suppose it might be beneficial to explain what architecture we do use, as an alternative to ViewModels.

We currently have an optional layer in our system called UI orchestration. Into this layer I ask developers to put the procedural code associated with carrying out an action/operation, which normally does more than just interact with a single CSLA object. Sometimes, we have unit of work classes to bring together the saving of multiple CSLA objects under a single transaction, although that is rarer, as that generally suggests we are reusing classes. However, as we split our systems up into system-specific and generic components (for reuse across multiple systems) there are still times when this seems the best approach.

We do, at least, recognise that (the modern equivalent of) code-behind isn't the only place to put the procedural code associated with performing UI actions!

Deluded perhaps, but sanity is just about maintained, in general. ;)

"If we had a bit more time, we could definitely do a better job." My name is Mort, and I'm addicted to weighing up the pros and cons of the approaches available, constantly walking the tightrope between what is right and what the uninitiated in our business think right looks like!

rockfordlhotka commented 5 years ago

It is important to understand that CSLA viewmodels aren't like traditional heavyweight viewmodels.

A traditional viewmodel implements the properties required by the UI, acting under the assumption that the entity model is in the wrong shape (i.e. doesn't expose the right properties for binding).

But a well-designed CSLA domain type does expose the right properties. So it would be a total waste of time/energy/testing/maintaining to reimplement all the same properties again in a viewmodel. Arg!!

So the CSLA viewmodel base types support the idea of a viewmodel having a Model property to which the UI binds. And the viewmodel exposes actions that the UI might trigger, such as refresh, save, cancel, navigate home, etc.

My suspicion is that what you are calling UI orchestration is terribly similar to a typical CSLA viewmodel.

rockfordlhotka commented 5 years ago

On a different topic - I did implement a PropertyInfo type for Blazor. It should be entirely reusable, you just need to provide it with the model and property name and it does the work for you.

PropertyInfo.cs

TheCakeMonster commented 5 years ago

I hear a noise; I think it might be the penny finally dropping!

One of the reasons I haven't recognised all that much benefit in MVVM is because I've been thinking of the ViewModel as transient. I've been having to deal with request/response for so long that my thinking has been clouded by it. Because rich client UIs don't suffer from the limitations of stateless request/response we've seen in the past in web development, I can see that there would be more value in spinning up another object, as it has a much longer lifetime.

I think Blazor needs an even greater change in mindset than I had realised, but I think I'm getting there.

rockfordlhotka commented 5 years ago

The MVVM design pattern got its start in WPF thanks to the rich data binding and a concept called commanding that allows UI controls to have events bound to an action. That means that the viewmodel reshapes data for the UI and implements bindable actions.

The web world looked at MVVM and found the ability to reshape the model for the UI valuable - at least within the context of Razor markup. But the web world didn't need the ability to bind actions because it is a postback-driven environment and because controllers already do that. So the web world only got half the value.

In any case, CSLA users have never needed the ability to reshape the model for the UI. That's always been a value proposition for CSLA, is that the model is already in the right shape. So if you use CSLA and MVC there's zero value to a viewmodel. However, in the XAML environment the ability to bind actions remained valid.

In WPF it is literally possible to write zero codebehind, because binding/commanding are powerful enough that actions are in the viewmodel and all rules/etc. are in the CSLA domain model.

In UWP it isn't quite possible because commanding isn't as powerful (last I looked anyway).

Blazor has (imo) an even better event binding model than WPF, plus good data binding. So to my way of thinking the MVVM pattern should be at least as good for Blazor as for WPF.

However, UI helper controls like PropertyStatus and PropertyInfo are invaluable in WPF, and I suspect the same is true for Blazor.

TheCakeMonster commented 5 years ago

Yeah, interesting. I'd already decided that avoiding the @code block was the way forward, and that the class over which you therefore base your component shouldn't be called Base, but instead something else. Like, maybe Actions, because what it would contain is action-like methods.

My word, in hindsight, it really seems like I'm putting in an awful lot of effort to avoid MVVM!!

Maybe, just maybe, you can teach an old dog a small tweak to a trick they always kinda knew, but wanted to call something else! Haha.

TheCakeMonster commented 5 years ago

This weekend (or some of it, at least) I think the plan might be to create a few proof of concept components, teasing out where I think we might be going for wider, more far-reaching aspect of this issue. It will be good to show some possible options for us to point at and discuss.

I think I can also mock up the original component I had in mind for this specific issue, as I think that has value in building out the toolbox of components that people can use in combination, if they so choose.

After having kicked around a few ideas, what I'm starting to settle on is that the component should be fairly straightforward, with a limited number of options. The more complex it is, the more people might struggle with it. A separate, more advanced offering is probably better than over-complicating a single component.

TheCakeMonster commented 5 years ago

First version is committed to my repo. I'm getting some advice from our UI expert on whether it offers the right HTML structure to make it easily stylable. We're also thinking about the accessibility of it.

If you'd like a preview, feel free to have a look there.

Here is the razor markup that represents an edit form using this component, in combination with the CslaValidator component, plus the built-in EditForm and InputText components:

<EditForm Model="_person" OnValidSubmit="Submit"> <CheckRulesValidator /> <div> <label for="firstname">First Name: </label> <InputText Id="firstname" @bind-Value="_person.FirstName" /> <BusinessRulesValidationMessages PropertyName="@nameof(_person.FirstName)" /> </div> <div> <label for="lastname">Last Name: </label> <InputText Id="lastname" @bind-Value="_person.LastName" /> <BusinessRulesValidationMessages PropertyName="@nameof(_person.LastName)" /> </div> <div> <button type="submit">Submit</button> </div> `

In this version, I've chosen to use property name as a parameter, rather than the lambda expression syntax needed to use the built-in component, as I think this is easier to type and understand.

TheCakeMonster commented 5 years ago

At the moment, this doesn't take benefit of the PropertyInfo class. That class exposes messages as a single, comma-separated string per type, whereas the component this replaces outputs a separate div per message.

TheCakeMonster commented 5 years ago

There is a lot more we could do to reduce the duplication of markup per property, as well as adding extra functionality built in. If we went further, then PropertyInfo would be much more useful.

My guess is that a more compact component might be of limited interest to a lot of advanced web developers. Generally, these people seem to feel it is their right to have full control over the markup that is exposed. However, that doesn't necessarily mean that we shouldn't provide it as an option, for those who don't want that level of control.

Blazor's templating supports the template being optional. That means a more advanced component could be created that offers a default input control - a textbox - but can be overridden. I don't see much advantage in being able to customise the label or the validation message component - if someone wants that much control then they will probably just specify their own markup; at that point, the wrapper component isn't offering much benefit.

rockfordlhotka commented 5 years ago

Please go ahead and submit a PR, that makes it easy to review and comment as the code evolves.

TheCakeMonster commented 5 years ago

I'm starting to worry that I've got my whole strategy wrong here. Not only that, but I think you might have been suggesting that for days, and I've not entirely picked up on it.

I can increasingly see that we have been coming at this from two different directions.

Your approach in BlazorExample is to rely on CSLA to do all of the work. Because of what exists in CSLA, that results in really simple markup. It's very straightforward.

My approach has been to adapt the ASP.NET team's architecture to fit against CSLA. They need Blazor to do more work, because they don't have a business rules engine that runs rules as properties are updated. As a result, they have created a number of components to make all of that happen. What I am trying to do is minimise the differences between Blazor with CSLA, and Blazor without, for a couple of reasons:

1) Standard Blazor code samples will be very similar to working code in Blazor+CSLA, so there might be less friction for those learning Blazor. 2) By being compatible with the framework, we're more likely to remain compatible with what third-party component vendors do.

However, your approach is simpler, needs fewer components, and is probably more similar to how people will work in other rich client frameworks. Furthermore, what we need from third-party component vendors will probably work your way as well.

What I think vendors are most likely to offer that will be of use to people is input controls - lots and lots of input controls. It seems likely that they will offer those through binding, because that's what the ASP.NET team have done. As long as these components are built in a way that doesn't require them to sit inside an EditForm, and thus assume that EditContext will be optional, then they will continue to work. I suppose that's reasonably likely, but not a definite.

Both approaches have some merit, but I now wonder if yours has more merit. It's a bolder vision, but perhaps a more daring approach, because it risks us being incompatible with what other people do.

Yours makes the existing Blazor edit components largely irrelevant, and thus the ValidationMessage replacement I have created isn't quite right. Mine is reliant upon being inside EditForm, and thus having EditContext available to it.

Maybe I should consider trying to adjust mine to work in either scenario, at the very least - if not drop the interaction with EditForm altogether.

There's one main difficulty with dropping EditForm, and that's the submit behaviour. At the moment, validation messages are only shown after a field has been edited, and I've managed to mimic that, more or less - but I rely on EditContext to do that. This is especially true for one scenario - when the submit button is pressed while the object is invalid, which is supposed to have the behaviour of showing all of he validation messages for all fields.

Hmm, I wonder if I can tweak CslaValidator to help me achieve that instead. Yeah, I might think on that some more, that might just work.

rockfordlhotka commented 5 years ago

@TheCakeMonster I think this does need more research and thought, and I don't want to have the pressure of getting it done before 23 Sept - I want us to build a good solution. So I'm going to remove it from the 5.0.0 project and we can add it in a point release when we feel it is complete.

rockfordlhotka commented 5 years ago

@TheCakeMonster I haven't tried using this editform concept. But if the Blazor team is pushing it then I'd like to at least have a nod of support for it, because that's where a lot of people will probably start.

But they really are waiting for a submit button before running rules? That's amazing to me, given that this is a smart client technology, and that submit button approach is (for the web) so late-1990's and (for the mainframe) so 1980's (when it was called a transmit key)...

I guess I can see where they are doing that, due to server-side Blazor. They can probably wait to transmit the form's data to the server for validation as a batch.

But with wasm you can get keystroke level interaction, which is what I'm doing in the BlazorExample sample. That has to be the goal, because that's a major value of using client-side computing, is to get rich user interaction.

TheCakeMonster commented 5 years ago

Ah, no, I may have misled you. The EditForm construct updates validation messages/status per-property, and probably even per-keystroke. However, I agree - over a WebSocket, per-keystroke might not be very reliable.

My comment about the submit button was about that fact that it also supports an object-wide revalidation, when you hit submit.

The general style is that it only reveals validation failures after a property has been updated, and not before. It's a case of "don't shout at the user until they've done something wrong." I think this is standard on the web. However, when you press the submit button, it revalidates everything, and shows ALL outstanding messages, on all fields.

[It appears that the recommendation from an accessibility perspective is that you do not disable the submit button when the form is invalid, because a person with limited visual acuity can get confused by this. This is taken as read in the remainder of the comment]

In CSLA, the current state of the rules exist all of the time - at least in my objects (a subject for another time, maybe.) Showing the messages only as they complete one field is very slightly tricky, but possible. However, there's a very specific implementation detail in how the submit button is handled that made replicating that very specific bit of functionality a bit harder.

The implementation detail is that the EditContext object transmits validation status changes via an event/delegate, but the exact same event/delegate is used for both per-property and full model revalidation, and there are exactly zero differences in the arguments between the two!

In the fullness of time, that might be something I submit a PR for against the aspnetcore repo. It seems a little unnecessary, and fixing it seems really easy to me.

TheCakeMonster commented 5 years ago

@TheCakeMonster I think this does need more research and thought, and I don't want to have the pressure of getting it done before 23 Sept - I want us to build a good solution. So I'm going to remove it from the 5.0.0 project and we can add it in a point release when we feel it is complete.

Agreed; good choice. I'm new to OSS contribution, but I'm sure that, just like public web APIs, it's a case of "commit in haste, repent at leisure!"

TheCakeMonster commented 4 years ago

The first version of this component is now committed, but whether it is the best it can be is still up for debate.

In developing the component, I have followed standard Blazor practices as far as possible, but for one opinionated decision that should be discussed at a bit more length.

Standard Blazor practice (and the choice of the ValidationMessage component built into .NET Core) is to expose a parameter named For, and to pass the property to be validated using a lambda expression. This isn't necessary with CSLA, as it contains great support for validating a property by name. As I think the lambda expression is slightly ugly and not very developer friendly, I chose instead to expose a parameter named PropertyName, and pass the name of the property as a text string. This can be done using nameof() if required, in case the name of the property is changed.

If you use the standard EditForm component, then this approach works really nicely. However, it does require that you use the CslaValidationMessages component inside of an EditForm. Using an EditForm is exactly what all Blazor edit samples will show on the web, so it seemed a reasonable approach at first. However, it isn't strictly required. As the current BlazorExample code in this repo demonstrates, CSLA contains enough functionality to work pretty well without EditForm.

If we chose to change CslaValidationMessages to support validation without an EditForm component, Blazor edit components using CSLA could be much simpler. However, they would not be the same as components that don't use CSLA, and this might be confusing to people who are new to CSLA, or Blazor. Furthermore, it is likely to complicate the CslaValidationMessages component, adding cost to the maintenance of CSLA into the future.

TheCakeMonster commented 4 years ago

Here is snippet of some sample markup for the edit of a basic Person object (incomplete, for brevity)

<EditForm Model="_person" OnValidSubmit="Submit">
    <CslaValidator />

    <div>
        <label for="firstname">First Name: </label>
        <InputText Id="firstname" @bind-Value="_person.FirstName" aria-describedby="firstNameMessages" />
        <CslaValidationMessages WrapperId="firstNameMessages" PropertyName="@nameof(_person.FirstName)" />
    </div>
    <div>
        <label for="lastname">Last Name: </label>
        <InputText Id="lastname" @bind-Value="_person.LastName" aria-describedby="lastNameMessages" />
        <CslaValidationMessages WrapperId="lastNameMessages" PropertyName="@nameof(_person.LastName)" />
    </div>
    <div>
        <button type="submit">Submit</button>
    </div>
</EditForm>
github-actions[bot] commented 2 years ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.