✨ amp-autocomplete: Support SSR templating #25260
Conversation
Test
|
|
||
| this.filter_ = userAssert( | ||
| this.element.getAttribute('filter'), | ||
| `${TAG} requires "filter" attribute.` |
There was a problem hiding this comment.
filter doesn't default to "NONE" if the attribute is not present?
There was a problem hiding this comment.
No. The filter attribute specifies a type of client-side filtering, so it is better if the publisher explicitly identifies when they don't want that. If we default to some value silently, it could be hard to debug an undesirable behavior here.
There was a problem hiding this comment.
Should the attribute be mandatory: true in validation then?
There was a problem hiding this comment.
Yes, that sounds like a reasonable change to me. Would you prefer that in this PR or in a follow up?
The reason that it wasn't originally marked as mandatory in the validations was because it was assumed it could be enforced here for both valid and invalid AMP pages at runtime.
| * @return {!Promise} | ||
| * @private | ||
| */ | ||
| autocomplete_(data, opt_input = '') { |
There was a problem hiding this comment.
This looks better, thanks. For future work: should we add user messaging for the failure cases below?
There was a problem hiding this comment.
Good idea, that is more in line with how other components behave, esp regarding data['html']
| } | ||
|
|
||
| const ampAutocomplete = new AmpAutocomplete(element); | ||
| const ssrTemplateHelper = ampAutocomplete.getSsrTemplateHelper(); |
There was a problem hiding this comment.
Heh, now that we fixed ssrTemplateHelper_ I noticed a lot of usage of other private vars in this test file.
No action needed but note for the future.
* First pass * Will comments Test * Replace template userAssert() with user().warn() * Add specificity to maybeFindTemplate querySelector * Remove template check in buildCallback * Support autocomplete in examples/viewer.html * Fix check-types issues * Add unit tests * Fix visual test * Will comments * Treat ssr isSupported() like a Promise * Don't treat SSR like a promise * . * Fix indentation * Refactor assertions * s/r/request * Refactor tests * SSR note * Type check * Oops more type check * Fix indentations * Tests with length * Sepand remaining comments * Restructure SSR-related assertions in build * Change data-value assertion to warning * . * Disallow filter attribute for ssr * Intermediary `autocomplete_` function * Update comment * Modify unit tests * Fix test after merge * Alphabetize imports * Stub ssrTemplateHelper
* First pass * Will comments Test * Replace template userAssert() with user().warn() * Add specificity to maybeFindTemplate querySelector * Remove template check in buildCallback * Support autocomplete in examples/viewer.html * Fix check-types issues * Add unit tests * Fix visual test * Will comments * Treat ssr isSupported() like a Promise * Don't treat SSR like a promise * . * Fix indentation * Refactor assertions * s/r/request * Refactor tests * SSR note * Type check * Oops more type check * Fix indentations * Tests with length * Sepand remaining comments * Restructure SSR-related assertions in build * Change data-value assertion to warning * . * Disallow filter attribute for ssr * Intermediary `autocomplete_` function * Update comment * Modify unit tests * Fix test after merge * Alphabetize imports * Stub ssrTemplateHelper
If server-side rendering is supported when there is a template provided,
amp-autocompletewill proxy the XHR for remote data and render the response. This change is one in a series to make it possible to supportamp-autocompletein AMP for Email #24881 cc/ @ampproject/wg-amp4emailNote: When SSR is supported, client-side filtering is disabled.
TODO