MudBlazor icon indicating copy to clipboard operation
MudBlazor copied to clipboard

MudBaseInput: The validation is only triggered if the field is dirty…

Open Schaeri opened this issue 4 years ago • 5 comments

Description

The problem we had in our project is that the validation should only be triggered when the input value of a form input element has been changed by the user (it is dirty). Currently, the validation is triggered when the user clicks into the input element and leaves it without any modification. This Pull Request resolves the issue described in #2879.

I introduced the parameter OnlyValidateIfDirty on the MudBaseInput class and a private field _isDirty. The flag (_isDirty) is set to true when the value of the input element has been changed by the user (SetValueAsync method). With this change, the on blur event only calls validate and set touched to true when the parameter OnlyValidateIfDirty is false or the is dirty flag was set to true.

How Has This Been Tested?

I added a new unit test for the TextField component which verifies the new and old behavior.
We included the new DLL into our project and verifed the old and new behavior by manually testing the feature. The functionality should not change the current behavior when the OnlyValidateIfDirty parameter is set to false, which is the default.

Types of changes

  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [x] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • [x] The PR is submitted to the correct branch (dev).
  • [x] My code follows the code style of this project.
  • [x] I've added relevant tests.

Schaeri avatar Jan 13 '22 16:01 Schaeri

Codecov Report

Merging #3726 (63ff54c) into dev (d55e31c) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##              dev    #3726   +/-   ##
=======================================
  Coverage   90.03%   90.04%           
=======================================
  Files         371      371           
  Lines       13382    13385    +3     
=======================================
+ Hits        12049    12052    +3     
  Misses       1333     1333           
Impacted Files Coverage Δ
src/MudBlazor/Base/MudBaseInput.cs 91.66% <100.00%> (+0.26%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

codecov[bot] avatar Jan 13 '22 17:01 codecov[bot]

We are currently having the exact same issue in our project with "not dirty" textfields that still get validated. Will this PR make it into the next MudBlazor update or should we look into a workaround?

Cerberus4444 avatar Feb 03 '22 19:02 Cerberus4444

I've lately had a very similar problem. only trigger validation if not empty or whitespace, but in fact I prefere the concept of only validating dirty fields.

Good work, thanks @Schaeri, I hope this gets integrated soon.

Schuttrim avatar Feb 08 '22 07:02 Schuttrim

Are there any plans to merge back this pull request?

If this change would not fit into the mudblazor design concept, then a feedback would be helpful. That way I could modify the code to suit all project requirements. Or if such a feature has no chance of getting into mudblazor, this information would be helpful too. In order to decide what to do next in our current project.

We waiting for feedback. Thanks a lot.

Schaeri avatar Mar 18 '22 07:03 Schaeri

Hello @just-the-benno

Thanks for the feedback and for testing the new feature. I was aware of the inconsistency, better said we also have no requirement in our project to change the validation behavior dynamically. The flag OnlyValidateIfDirty is for us simply a static configuration that is set once in the page design. In our use case always True.

It is now a requirement question how the UI element in MudBlazor should behave for all projects. If you find, the flag must also be able to be set dynamically, at any time, then I will try to fulfill this requirement. And revise the implementation and tests again. Then the question is how the UI element should behave. Just a reset of the validation? Or resetting the UI element to the initial state. So also deleting the input?

Depending on the requirement I will then also try to map the feature as suggested in the documentation.

If the implementation is left as is I would include two elements in one form and also describe the restrictions. If this is ok for you in a separate chapter in the form validation page of the documentation.

If I revise the implementation again, then I will include the feature as suggested directly in the Simple Form Validation example.

Schaeri avatar Aug 10 '22 20:08 Schaeri

Hi @Schaeri

Thanks for your thoughts. I wanted to bring it to your attention. I think in most applicat,ions this value is not dynamic so it is - in my view - acceptable to not cater for changes OnlyValidateIfDirty during the lifetime of the component. If the need arises in the future, our "future self" can worry about it. :)

To merge your PR finally into the code base, please provide a simple (static) example, and we are good to go.

just-the-benno avatar Aug 11 '22 07:08 just-the-benno

Hi @just-the-benno I have created the example.

Schaeri avatar Aug 11 '22 13:08 Schaeri

Looking at the code I see that _isDirty is never ever set to false except at initialization. Shouldn't a Reset() reset it, or am I missing something?

henon avatar Aug 14 '22 10:08 henon

If a user decides to reset a form, I'd argue that they have still interacted with a component before (or not), so the reset itself wouldn't change the _isDirty state.

If I understand the PR correctly, the intention here is to prevent showing validation messages for fields when a user hasn't interacted with them previously. And hence, you don't see _isDirty = false anywhere because this would mean to undo the interaction.

What are your thoughts, @Schaeri, regarding reset?

just-the-benno avatar Aug 14 '22 21:08 just-the-benno

Hello @just-the-benno, Hello @henon

As @just-the-benno explained this execution is correct for me. The flag stores if a user made a manipulation on the input element or not. If this was not the case no validation is called in case of OnlyValidateIfDirty="true".

The question if Reset resets this state or leaves it is again a requirement question for me. I also don't know in which situations the reset is called. Is this just something manual that doesn't happen in the framework code? Or does the framework also call a reset in certain situations?

In our use case we never call a reset. Thus, unfortunately, we have not considered the case. If the requirement is that a reset resets the control to its original state, i.e. sets the initial default value and also resets the validation then one can also argue that the user manipulation should be reset.

As long as our requirement is not violated I can adjust the reset behavior to your wishes. And test if there are no side effects for us.

Please give me feedback if I should change the reset behavior.

Schaeri avatar Aug 16 '22 06:08 Schaeri

Reset will only be called manually. It allows to reset the form into its initial untouched state. This is of course most useful for our documentation examples but I am sure there are also sensible use-cases of it in the wild.

So I think reset should also reset the flag.

henon avatar Aug 16 '22 06:08 henon

Hi @Schaeri

Based on @henon suggestions, I think this is the last and final change before merging. Can you please implement the logic that set the value of _isDirty to false in case a reset is requested? You can either test it with a new test or include this in your current "flow" test.

just-the-benno avatar Aug 16 '22 07:08 just-the-benno

@just-the-benno: Thanks for the help. I implemented the rest and adapted the test.

Schaeri avatar Aug 16 '22 08:08 Schaeri

Thanks a lot!

henon avatar Aug 16 '22 08:08 henon

I'm glad that your changes could finally make it into the project. And I'm sorry that we didn't start earlier to work together.

just-the-benno avatar Aug 16 '22 15:08 just-the-benno