Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Inline form field validation #4034

Closed
wants to merge 23 commits into from
Closed

[WIP] Inline form field validation #4034

wants to merge 23 commits into from

Conversation

bennothommo
Copy link
Contributor

@bennothommo bennothommo commented Jan 4, 2019

This is a WIP / Proposal to implement inline form field validation for forms built with the Form widget.

One of the shortcomings of validation in October currently is that the form is validated on submission, and only one error message is shown via a flash message. While this is fine for smaller forms, when larger forms are produced, and the possibility of multiple fields having issues increases, it could cause frustration getting messages one at a time.

This pull request aims to provide immediate feedback on errors with form field contents by:

  • Adding a validation call when a field has lost focus or has changed.
  • Validating the linked model for the current form and retrieving any errors for the changed input in the context of the entire form contents. (ie. if a field is invalid due to the contents of another field, this too needs to be detected)
  • Showing the validation error in a friendly way to the user (perhaps as a message underneath the field, or as an icon next to the label and using a tooltip message to display the error)

At the moment, the scope is to only handle simple form fields, such as text inputs and textareas, select dropdowns, radio options and checkboxes, but could potentially extend to more advanced widgets if they can be easily validated in the context of a model.

Tasks:

  • Add onValidate event for fields in Widget
  • Add JS bindings for all simple types of form fields
    • Text-field based fields (text, textarea, number and password)
    • Dropdowns and balloon selectors
    • Checkboxes, switches and checkbox lists
    • Radio lists
  • Add error messages to form
  • Prevent form submission if any fields have errors
  • Add boolean to form_config.yaml to enable/disable validation
  • Add hooks to allow plugins to extend field validation
  • Add unit tests

Copy link
Contributor

@LukeTowers LukeTowers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks pretty nifty, just a few comments about JS style

modules/backend/widgets/form/assets/js/october.form.js Outdated Show resolved Hide resolved
modules/backend/widgets/form/assets/js/october.form.js Outdated Show resolved Hide resolved
modules/backend/widgets/form/assets/js/october.form.js Outdated Show resolved Hide resolved
modules/backend/widgets/form/assets/js/october.form.js Outdated Show resolved Hide resolved
modules/backend/widgets/form/assets/js/october.form.js Outdated Show resolved Hide resolved
modules/backend/widgets/form/assets/js/october.form.js Outdated Show resolved Hide resolved
modules/backend/widgets/Form.php Outdated Show resolved Hide resolved
@LukeTowers
Copy link
Contributor

Do you want to post some screenshots of the different options of displaying the error messages to the user? Keep in mind that not all fields have labels all of the time and we'd need it to work well on mobile too.

@bennothommo
Copy link
Contributor Author

@LukeTowers Not a problem. Will try and mock those up over the weekend.

@bennothommo
Copy link
Contributor Author

Here are three options that I can think of so far:

The simple inline error message:

image

Should work with all types of fields, with the only downside being that it could considerably shift the layout of the form if there are quite a few fields that have errors.

Or, there could be an icon with a tooltip message that is shown on mouse hover (or by tapping the icon on mobile):

image

This would minimise the real estate needed for error messages and keep the form looking "clean". The only wrinkles would be working out placement when the label is missing, and for inline fields such as radio and checkboxes.

Finally, there could be a tooltip shown when the field is focused:

image

Removes the need for an icon and finding placement for it, but again, might be difficult to show for radios and checkboxes because they only get focused on when clicked.

I personally find the icon with tooltip popup to be the most "clean" way of showing the error (I've been using it so far for my own local copy), but ultimately, it may need to be the simple inline error message to support all types of fields.

@w20k
Copy link
Contributor

w20k commented Jan 6, 2019

@bennothommo, ping me when you need major testing to be done 😉

@bennothommo
Copy link
Contributor Author

@w20k Will do 😃

@bennothommo
Copy link
Contributor Author

@LukeTowers Do you have any thoughts on the styling of the error messages per #4034 (comment)? I might try and finalise that first before hooking it up to the other field types.

@LukeTowers
Copy link
Contributor

LukeTowers commented Jan 7, 2019

@bennothommo I'm leaning towards the inline messages right now, they're the simplest for the user to understand what's going on, perhaps with some iconography for colour accessibility? This article makes a lot of excellent points. Perhaps look at this one too: https://designmodo.com/ux-form-validation/

@bennothommo
Copy link
Contributor Author

@LukeTowers Thanks for that. I think you're right, and those articles make compelling points. I'll proceed with the simple error message underneath the field.

@w20k
Copy link
Contributor

w20k commented Jan 7, 2019

@bennothommo, would be awesome if you add JUnit tests 😄

@bennothommo
Copy link
Contributor Author

@w20k Sure thing. Is that similar to the functional tests in the tests directory, or is that something else?

@w20k
Copy link
Contributor

w20k commented Jan 7, 2019

@bennothommo, I think unit tests would work. Don't feel like functional testing is needed here.

@w20k
Copy link
Contributor

w20k commented May 3, 2019

@bennothommo do you think I could be of any use? Would love to help you to finish this PR ;)

@bennothommo
Copy link
Contributor Author

bennothommo commented May 4, 2019

@w20k I'd love any help you're willing to provide :). What would you be interested in having a look at? You could either set up some handlers for checkboxes and radio inputs, or you could maybe set up a PR for the test plugin to test this feature? Essentially, to test this feature, you just need to add inlineValidation: true to any form config or field config inside a form config to enable the check.

If you wanted to do some work on this plugin, it may be best to set up a PR to go to my branch above.

@bennothommo bennothommo self-assigned this May 4, 2019
@w20k
Copy link
Contributor

w20k commented May 4, 2019

@bennothommo I'll start from making a PR to the test-plugin. So, that I would be easier to test, already implemented features, and understand what's there missing :)

Would also ask you to push your branch to OctoberCore, if @LukeTowers don't mind. So, that I could provide PR's to help you with the coding part as well. Little by little getting back to my daily duties 😄

widget = this

var $innerField = $(field.querySelector('select'))
if (!$innerField) {
Copy link
Contributor

@w20k w20k May 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bennothommo I think you have a bug :) $innerField will be an empty object, but present.
You could either change it to !!$innerField or $innerField.length === 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch :)

continue
}

var fieldClasses = field.className.split(/\s+/).map(function (className) {
Copy link
Contributor

@w20k w20k May 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bennothommo I think this part is overly complicated :)
If I've understood it correctly you need to find the field which has a class corresponded to the Regexp - /^([a-z\-]+)-field$/ and that's it.

Two options: either use indexOf() to cut down all those not needed fields or change Regex to be more accurate (before&after spaces, dirty try: (\s+)?([a-z\-]+)-field(\s+)? ).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@w20k Yeah I did initially have just a regex, but I thought this way was more readable, albeit a bit more complex. Your regex works just as well though. :)

@bennothommo
Copy link
Contributor Author

This PR is now being developed in #4322.

@bennothommo bennothommo closed this May 7, 2019
@bennothommo bennothommo deleted the form-field-validation branch May 7, 2019 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants