[release-10.1] Fix form validation #4227
Conversation
The old validation mechanism was not compatible with Bootstrap 5 and didn't work properly with Bootstrap 3 either. The new uses the JS validation API. It is theme-independent and less intrusive with the caveat that the validation error messages are displayed in a browser-specific style.
There was a problem hiding this comment.
Thanks, @EreMaijala, this is off to a good start. Looks like one test needs adjustment, though; I'm getting this failure in my environment:
There was 1 failure:
1) VuFindTest\Mink\RecordActionsTest::testSMS
Element not found: .modal .sms-error index 0
Failed asserting that null is of type object.
/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:528
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/RecordActionsTest.php:705
Also, see below for a couple of other thoughts.
| function checkPhoneNumberValidity(ev) { | ||
| const field = ev.target; | ||
| if (field.id && field.type === 'tel' && field.dataset.validatorRegion) { | ||
| const valid = isPhoneNumberValid(field.value, field.dataset.validatorRegion); |
There was a problem hiding this comment.
If I'm following things correctly, I believe that isPhoneNumberValid is being exported by libphonenumber. It looks to me like we're using a version of libphonenumber from 2010 or so, which is probably outdated and inaccurate. It's out of scope for the current PR, but maybe when we update this for the 11.0 release, we should also introduce loading of libphonenumber via NPM (possibly using the lighter-weight libphonenumber-js) and make sure things work with the latest version. What do you think?
There was a problem hiding this comment.
That all would make sense, but I'd handle it separately. Not sure if libphonenumber-js supports usage without a module system, but any update would probably be better than the current one.
There was a problem hiding this comment.
Agreed, definitely a separate issue, but worth doing before we forget about it. I can open a JIRA ticket if you think that would be the best next step.
There was a problem hiding this comment.
Maybe it would even make sense to evaluate all of our JS dependencies that are not currently installed via NPM and open tickets to update/replace all of them.
There was a problem hiding this comment.
Sounds good, please open a ticket about it!
|
@demiankatz Test fixed. And the |
There was a problem hiding this comment.
Thanks, @EreMaijala -- looks good to me now! A few tests were accidentally disabled by a recent commit, but I just went ahead and fixed that. Everything is passing on my end.
The old validation mechanism was not compatible with Bootstrap 5 and didn't work properly with Bootstrap 3 either. The new uses the JS validation API. It is theme-independent and less intrusive with the caveat that the validation error messages are displayed in a browser-specific style. (cherry picked from commit 9f0985e)
The old validation mechanism was not compatible with Bootstrap 5 and didn't work properly with Bootstrap 3 either. The new uses the JS validation API. It is theme-independent and less intrusive with the caveat that the validation error messages are displayed in a browser-specific style. (cherry picked from commit 9f0985e) (cherry picked from commit 8d3cc7f)
The old validation mechanism was not compatible with Bootstrap 5 and didn't work properly with Bootstrap 3 either. The new uses the JS validation API. It is theme-independent and less intrusive with the caveat that the validation error messages are displayed in a browser-specific style.
TODO