Allow basic usage of form and polyfill validate check globally #3373
Conversation
| /** | ||
| * @param {!Window} window | ||
| */ | ||
| export function uninstallGlobalSubmitListener(window) { |
There was a problem hiding this comment.
We don't seem to use this?
Could we simplify this file and get rid of the service?
There was a problem hiding this comment.
Do we just store a local variable in this file and memoize it? How do we handle multiple window situations? Would something like the following be ok?
/**
* @type {?SubmitHandler} Store the global handler for form submits.
*/
let submitHandler_;
/**
* @param {!Window} window
*/
export function installGlobalSubmitListener(window) {
submitHandlerFor(window);
}
/**
* @param {!Window} window
*/
function submitHandlerFor(window) {
return submitHandler_ || (submitHandler_ = new SubmitHandler(window));
}There was a problem hiding this comment.
@cramforce let me know if the above is good or if we handle this in a different way?
There was a problem hiding this comment.
Done. Let me know if this looks good.
As per chat, we only need to install this once on the passed window.
| // available per input.validity object. We need to figure out a way of | ||
| // displaying these. | ||
| if (form.checkValidity && !form.checkValidity()) { | ||
| form.classList.remove('amp-form-valid'); |
There was a problem hiding this comment.
So, when we check for validity via form.checkValidity - does Safari set :invalid on the form itself? If so, I'd like to defer our own classes until the next PR.
There was a problem hiding this comment.
:invalid would be applied on load to the form not on submit. Happy to drop all the classes from this PR.
There was a problem hiding this comment.
Let's do. I think it'd be easier to add them in one go later.
|
@dvoytenko @cramforce thanks for all comments 😄 . PTAL 👀 |
| /** | ||
| * Removes all event listeners. | ||
| */ | ||
| cleanup() { |
There was a problem hiding this comment.
There is no caller of this. Can this entire class just be inlined into the function installGlobalSubmitListener
| /** | ||
| * @param {!Window} window | ||
| */ | ||
| function submitHandlerFor(window) { |
There was a problem hiding this comment.
This has been dropped.
|
PTAL 👀 |
|
LGTM |
| if (prefix.length > string.length) { | ||
| return false; | ||
| } | ||
| return string.indexOf(prefix) == 0; |
There was a problem hiding this comment.
We can optimize this using a rather nifty trick:
return string.lastIndexOf(prefix, 0) === 0;This prevents a full scan of the string, only checking index 0 (and possibly 1, 2, up to prefix.length if they match).
There was a problem hiding this comment.
Woah, that's pretty cool 👍 . Done.
Kick off form support #3343.
This is a prequel to #3344 as discussed to provide basic support for form before we introduce some more features.