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

Enhance HTML5 validation with JS #2021

Merged
merged 27 commits into from Jul 11, 2018

Conversation

Projects
None yet
3 participants
@hbillings
Member

hbillings commented Jun 22, 2018

Note: Against #2011, not develop. Should be merged before #2011 is.

Closes #2017.

To do:

  • Only show error message once for radios, checkboxes, and date widgets.
  • Finish styling the errors appropriately.
  • Tests

@hbillings hbillings changed the base branch from develop to 1697-alerts Jun 22, 2018

// or has a parent with the class usa-form-group
if (options.showErrorMsg) {
parent.insertBefore(errorContainer, fieldsetLabel);

This comment has been minimized.

@tbaxter-18f

tbaxter-18f Jun 22, 2018

Member

I'm not sure, but could this throw an undefined error if it didn't find a legend on L48?

This comment has been minimized.

@hbillings

hbillings Jun 22, 2018

Member

yep, sure could! I updated it to grab a label if there's no legend, which should always be truthy -- but I suppose there's still the possibility that an input could exist without either... maybe I should throw in a check to make sure things exist like I think they do in there.

});
// 'input' will fire each time the user types.
input.addEventListener('input', function () {

This comment has been minimized.

@tbaxter-18f

tbaxter-18f Jun 22, 2018

Member

What about textareas? And are we comfortable there won't be too much typing going on?

This comment has been minimized.

@hbillings

hbillings Jun 22, 2018

Member

input is just the name of the event in this case, which is weird. I actually removed this, because it causes jumpiness with our styles, so now we just check validity on submit.

// Applied by form-validation.js as an enhancement for html5 validation styles
.form--invalid {
border: 3px solid $color-red-dark;
border-left: 3px solid $color-red-dark;

This comment has been minimized.

@tbaxter-18f

tbaxter-18f Jun 22, 2018

Member

We don't need both these rules do we?

This comment has been minimized.

@hbillings

hbillings Jun 22, 2018

Member

ALL THE RED :all-the-things:

@ericronne ericronne self-requested a review Jun 22, 2018

@ericronne

Can't speak for circle ci, but it looks brilliant to me

@hbillings

This comment has been minimized.

Member

hbillings commented Jun 22, 2018

Okay, one weird thing that's happening (I suspect because I've got preventDefault on invalid events) is that the "Next" button only validates fields but doesn't submit them after the user clicks the button. The user has to click the button twice to submit the field. I'm sort of out of brainpower right now, but I suspect I need to listen for whatever event is emitted there to allow the submission to go through on the first click.

@hbillings hbillings changed the title from [WIP] Enhance HTML5 validation with JS to Enhance HTML5 validation with JS Jul 5, 2018

@hbillings

This comment has been minimized.

Member

hbillings commented Jul 5, 2018

@tbaxter-18f if you could review this again, 'twould be most appreciated!

// set the error message on that, otherwise we end up with multiple messages.
// Because we're using django-uswds-forms, every form should be encapsulated
// in a fieldset automatically.
export function findParentNode(node) {

This comment has been minimized.

@tbaxter-18f

tbaxter-18f Jul 6, 2018

Member

Just curious, why'd you decide to go this route rather than just node.closest('fieldset')?

This comment has been minimized.

@hbillings

hbillings Jul 6, 2018

Member

It needs a polyfill in IE11. :-(

export function documentCreateElement(window, type) {
return window.document.createElement(type);
}

This comment has been minimized.

@tbaxter-18f

tbaxter-18f Jul 6, 2018

Member

I don't think I'm clear on the need for this function?

This comment has been minimized.

@hbillings

hbillings Jul 6, 2018

Member

It's pretty much solely for testing, ugh. I'm doing so much with the DOM that I have to mock up a fake window in order to test all the methods I'm using. 🤦‍♀️

const errorContainer = parent.querySelector(`.${INVALID_MESSAGE_CLASS}`)
|| documentCreateElement(window, 'p');
// grouped inputs like radios have legends; single inputs just have labels
const fieldsetLabel = parent.getElementsByTagName('legend')[0] || parent.getElementsByTagName('label')[0];

This comment has been minimized.

@tbaxter-18f

tbaxter-18f Jul 6, 2018

Member

would parent.queryselector('legend') offer any advantage? Probably not. Just thinking out loud.

This comment has been minimized.

@hbillings

hbillings Jul 6, 2018

Member

Oh! Derp, yeah, that would probably be easier.

export function domContentLoaded(win) {
// there are several forms on the page; get the one within the .content div
// TODO: make this a more reliable ID selector or something

This comment has been minimized.

@tbaxter-18f

tbaxter-18f Jul 6, 2018

Member

Wouldn't you want to run this on all forms on a given page?

This comment has been minimized.

@hbillings

hbillings Jul 6, 2018

Member

The page I was looking at had four(!) form elements, one of which was the hidden form that contains the data that was already entered. That data should already be valid since it's run through this, but if it's not, there's no way to fix errors on the user's end.

This comment has been minimized.

@hbillings

hbillings Jul 6, 2018

Member

(That said, I'm not crazy about this solution and would be totes open to suggestions.)

This comment has been minimized.

@tbaxter-18f

tbaxter-18f Jul 6, 2018

Member

OK, just spitballing, but what about looping through form:visible?

Note: actual selector may vary.

This comment has been minimized.

@hbillings

hbillings Jul 10, 2018

Member

selectors in mirror are larger than they appear

hbillings added some commits Jul 11, 2018

@hbillings hbillings merged commit 19cf74f into 1697-alerts Jul 11, 2018

3 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
security/snyk - package.json (CALC) No manifest changes detected
security/snyk - requirements.txt (CALC) No manifest changes detected

@hbillings hbillings deleted the 2017-html5-styles branch Jul 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment