Design Review 2017-04-12 (amp-ad-exit, amp-form async validation, amp-sort)

[mrjoro]

The AMP Project holds weekly engineering design reviews on Wednesdays @ 1pm Pacific via video conference using Google Hangouts.

Participation by the entire AMP Project community is encouraged.

This issue will be updated by 1pm Pacific on the Monday before the design review with links to the design docs that will be discussed. Please read through the design docs that will be discussed before attending the design review.

If you have a design you'd like to bring to design review, please read through the guidelines. When you are ready to bring your design to design review (after following the guidelines) update the appropriate Design Review GitHub issue with a link to your design doc and a brief description by 1pm Pacific on Monday of the week you'd like to present your design.

[clawr]

I'll come to discuss #8515

[cvializ]

I have a design for asynchronous form validation that will be used to implement #8369

[dvoytenko]

I'd like to discuss responsive ads with width-based layout. Design.

[aghassemi]

Time permitting, @derekcecillewis and I like to talk about amp-sort proposal: #8691 (needs about ~15 mins)

[mrjoro]

Since we pushed out the design reviews for @clawr and @derekcecillewis from last week, let's cover those first this week. We'll do @cvializ and @dvoytenko's if time permits, else push those to next week.

[DerekNonGeneric]

It's unfortunate, but as I've mentioned, I have a schedule conflict with design reviews for the rest of this semester. As @aghassemi intends on proposing a separate component, mine will not be directly implicated. As much as I'd like to attend, it would mean being absent from a session that I cannot afford to miss. I hope that many of the design decisions that we worked through with amp-sortable-table can live on in amp-sort. If the design gets approval, feel free to close #6057. I'll keep a close eye on the progress of amp-sort and let you know if I have any concerns. Best wishes!

[mrjoro]

amp-ad-exit (#8515)

• will help developers with navigation from ads & provide a good user experience
• when the filter is matched, you don't actually execute the click? yes
• race condition: should we provide the link immediately so it can happen if the user clicks before the JS loads? most of the time we expect the JS to load within the 1 second we'd reject anyway, so preference would be to to be conservative and wait
• are we expecting more filters? probably over time, e.g. we could prompt the user "did you really mean to click"/click to proceed; it should be extensible enough to support them
• should target always be _blank? hopefully; or it could be configurable to use _top, but tracking URLs would be an issue
• often we have multiple parts of the ad that we want to go to the same URL (e.g. a carousel) + easier for the person constructing the ad to have one part that never changes (instead of going through the DOM and adding config where it fits); @camelburrito will document his idea in the issue
• could we use a global clickhandler to catch all the clicks and then annotate elements? @dvoytenko will add some detailed comments to the issue
• is this just for A4A or all of AMP; if it's more broadly useful we could add it elsewhere; is it a concern that users could ping back for clicks if used outside of ads? AMP already allows that; per @cramforce it might not be generally useful but wouldn't cause much harm
• should we name it something else if we want to make it more useful, e.g. amp-exit instead amp-ad-exit? @cramforce: name is fine
• if you're in a viewer you should use target _blank & we deny target _top; _blank is a fine default, developer can choose something else but it might get denied by the viewer/browser
• ad vendors (e.g. AdWords) may require specific target names in the exit-api.exit that developers then specify on their page somewhere, but that shouldn't be part of the amp-ad-exit spec

[mrjoro]

amp-form asynchronous validation (#8369)

• motivation was validating ZIP codes based on city name/rest of address which can't be done on the client
• would form request be JSON? probably can't do that with CORS; most consumers would be okay with default form request format; we already do form AJAX submissions so it's probably the same thing as that
• should we restrict async validation for the "as you go" case? instead of adding an extra call to when the form is submitted should we extend the regular form submit case to be able to perform the extra validation?
• what if we instead of having the developer specify parts of the form we always submit the entire form (with an indication that it's a pre-submit)?
  -- the benefit of the separate async approach is that it can work better with dedicated services (e.g. a dedicated ZIP code validator)
  -- but for complicated forms the extra identifiers/etc. can become tricky
  -- submitting the entire form wouldn't work well for image upload field
• there is a lot of prior art (e.g. jquery validation plugin), so we should make sure we aren't repeating any of the mistakes; it's actually quite hard to write a good validation framework
• we aren't targeting the autocomplete use case; we don't validate as the user is typing, only after they leave a field

[mrjoro]

amp-sort (#8691)

• originally a design for sortable tables, but realized maybe it could apply to things other than tables
• can you nest sorts? yes, or you can change what you're sorting by (via bind or at the time you call .sort())
• general mutation algorithm? insert before; did we try to minimize the elements that need to be moved? we haven't yet, but should make sure it's not an issue, and there's a benefit to a simple algorithm
• @dvoytenko: default should be descending order
• can you remove a sort (to get the default ordering back)? you'd have to remember the original order
• all children should have the data to be sortable, else they'll end up at the bottom of the sort
• what about nested sortable items? data attributes have to be on the direct children (i.e. all of the sortable items are siblings, but could be N levels down from the amp-sort as long as they are at the same level); do we enforce that? we should
• call it sort or sortable? sort is good