chanan / BlazorStrap

Bootstrap 4 Components for Blazor Framework
https://blazorstrap.io
The Unlicense
916 stars 157 forks source link

BSInputCheckbox and BSInputRadio do not trigger EditContext.OnFieldChanged when toggled #525

Closed ugumba closed 2 years ago

ugumba commented 2 years ago

EditContext.OnFieldChanged is called when focus changes, but not when the inputs are toggled. This is problematic for an EditForm which relies on this callback to be informed when anything changes within the Form.

InputRadio and InputCheckbox work fine in this respect.

uecasm commented 2 years ago

Just noticed this myself. You can disable the validation on blur with ValidateOnBlur="false" but then it's unclear whether the intended "validate on click" would be ValidateOnChange="true" or ValidateOnInput="true" (though the former seems more likely).

Having said that, it looks like ValidateOnChange is completely ignored at present (the OnChangeEvent actually checks ValidateOnInput, likely because OnInputEvent calls it too), and BSInputCheckbox/BSInputRadio don't wire up the onchange event anyway, and the click event doesn't do anything related. (FWIW, InputRadio uses onchange instead of onclick.)

ugumba commented 2 years ago

Thanks! Onchange is definitely the desired event (which would make onblur redundant), but it sounds like we have a workable (and hopefully temporary) workaround.

uecasm commented 2 years ago

I'm not sure what I said counts as a workaround; it's more of a problem statement. Unless you just meant using InputRadio instead.

ugumba commented 2 years ago

Yes, I was a bit hasty: I had assumed ValidateOnInput="true" would work, but it doesn't.

uecasm commented 2 years ago

Yeah, sorry, I should have been clearer about that. The only event that is currently wired up is onclick. oninput wouldn't work anyway since that's only really intended for text input. Sadly it doesn't expose an OnClick either, so you can't even do things that way.

Other than using InputRadio/InputCheckbox for now, the only workaround I can think of is to add the validation call in the bound property setter (which is a bit awkward since you don't usually have access to the EditContext on the "outside"; though not impossible).

jbomhold3 commented 2 years ago

Will address it when I get back from this business trip. Though @uecasm will have probably have a PR before the plane lands.

uecasm commented 2 years ago

I kinda deliberately didn't make a PR for this (yet) since I'm not entirely sure what the intended solution might be. There are a number of possibilities each with different caveats.

ugumba commented 2 years ago

Hi guys! What's the status on this issue?

jbomhold3 commented 2 years ago

Just added it in doing some testing now.

jbomhold3 commented 2 years ago

Tested working as intended. If ValidateOnChange or ValidateOnInput is true, it will now fire the moment the value is updated. So clicked in this case. Will be out in the next beta build.

ugumba commented 2 years ago

Thanks! So this would fire validation.
But OnFieldChanged should be called regardless of whether validation is enabled or not?

jbomhold3 commented 2 years ago

As written, as long as there is an EditContext and one of the two bools is true. It doesn't care if you have a validator in use.

jbomhold3 commented 2 years ago

https://gyazo.com/1fb0a9d71fc3a202a1258d391198e7c1

jbomhold3 commented 2 years ago

Just realized radios might double notify atm but an easy enough fix will add the logic if it does after I test it ~at work~ when I get back home.

jbomhold3 commented 2 years ago

Released in https://www.nuget.org/packages/BlazorStrap/5.1.100-Beta2. Please see the beta install issue for the install steps.