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
Refactor form verifiers API. #9798
Conversation
extensions/amp-form/0.1/amp-form.js
Outdated
/** @const @private {?string} */ | ||
this.xhrVerify_ = this.form_.getAttribute('verify-xhr'); | ||
if (this.xhrVerify_) { | ||
assertHttpsUrl(this.xhrVerify_, this.form_, 'verify-xhr'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's refactor to share validation logic between action-xhr
and this.
extensions/amp-form/0.1/amp-form.js
Outdated
this.form_.addEventListener('blur', e => { | ||
checkUserValidityAfterInteraction_(dev().assertElement(e.target)); | ||
this.validator_.onBlur(e); | ||
}, true); | ||
|
||
const cleanup = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cleanup
too generic given this methods deals with non-verifier code as well. Maybe afterVerifierCommit
?
extensions/amp-form/0.1/amp-form.js
Outdated
|
||
/** | ||
* Send a request to the form's verify endpoint. | ||
* @private |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add return type.
extensions/amp-form/0.1/amp-form.js
Outdated
* @param {string} url | ||
* @param {string} method | ||
* @param {!Object<string, string>=} opt_extraFields | ||
* @private |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto return type
isDirty_() { | ||
const form = this.form_; | ||
for (let i = 0; i < form.elements.length; i++) { | ||
const field = form.elements[i]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can break early for disable elements, they won't get submitted anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
isDirty_() { | ||
const form = this.form_; | ||
for (let i = 0; i < form.elements.length; i++) { | ||
const field = form.elements[i]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoo, this is going to be a slow loop. This'll recreate elements
HTMLFormControlsCollection on every iteration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I didn't consider it'd be computed every access. I'll cache it in a variable outside the loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implements the suggestions @dvoytenko gave for the form verification API.
verify-xhr
attribute functioning similarly toaction-xhr
change
event. The groups allowed us to limit the requests to only send when a group was completeCloses #9696