Design Review 2017-07-26 (sidebar child/width restrictions, theming via dynamic endpoint)

[mrjoro]

The AMP Project holds weekly engineering design reviews. We encourage everyone in the community to participate in these design reviews.

Time: Wednesdays @ 1pm Pacific
Location: Video conference via Google Hangouts

If you are interested in bringing your design to design review, read the design review documentation and add a link to your design doc by the Monday before your design review.

When attending a design review please read through the designs before the design review starts. This allows us to spend more time on discussion of the design.

If you are unable to make the 1pm Pacific time due to time zone issues but you have a design you would like to present please make a note of that and we will try to find a time that works.

[ericlindley-g]

Adding an item for quick discussion this coming week: #10587

[dvoytenko]

ITI dynamic theming endpoint: #10647. 20 min

[mrjoro]

remove sidebar child & width restrictions

• amp-sidebar can't be open on load, so less of a risk for things like full page ads in the sidebar (and there are other ways of accomplishing those problematic cases)
• @cramforce: I always thought this restriction didn't make any sense; was okay with conservative approach, but AMP has evolved a lot since the beginning; there's no reason to give the sidebar any particular restrictions; @jpettitt strongly agrees
• @jpettitt: should be able to make the background whatever you want
• @pbakaus : feature request is to be able take the sidebar and have a property (like media) so "for this particular resolution, style the sidebar differently" which would let him support pulling things out of the sidebar into the menu for desktop mode
  • the project @torch2424/@camelburrito are working on sort of solves this problem, but it is not exactly the solution Paul is looking for; that solution uses another amp component inside of the sidebar to indicate what gets pulled out into a menu, but Paul prefers to do the same thing with media queries (John agrees)
  • @dvoytenko: do we care about content duplication?
  • Paul's example for ampproject.org is fairly straightforward; John has some more complicated examples that we should consider
  • @ericlindley-g will update the issue to make it more broad to lead into a discussion with Paul/John/Sriram/Aaron/etc.
• John: perhaps amp-sidebar can appear anywhere in the document rather than a top level element? the reason for this was long problem with positioning on iOS
• part of the reason for the width restriction was we wanted to avoid people accidentally getting trapped with the sidebar open, but now there are many other ways to do something similar (e.g. with amp-bind) that it might not be something we want to enforce at the component level; sidebar always close on back button anyway
  • it's more of an issue of whether the sidebar opens automatically; if it only opens on user action than the problem is solved
• lifting the content & width restrictions makes sense; we'll revisit some of the other issues that came up

[mrjoro]

theming via dynamic endpoint

• why not do it in a more generic way with amp-access?
  • has a very specific goal (hides or shows sections) and is always render blocking
  • amp-access is not about personalization
  • personalization in this case comes from a logged in value
  • can't call this in pre-render, but the second time the value would be cached (a classic one behind)
  • it's bad for performance
  • we might be prerendering the wrong elements and then there's a delay when the user clicks on it
  • this is more of a documentation concern (with the publisher shooting themselves in the foot)
• for real time config for ads we do something similar to this (for ad tagging); went down a slightly different path (use a no cache header force update), then force skip client cache
• maybe provide a generic storage API with high entropy and a logout mechanism? no way to provide a logout mechanism in Safari (in Chrome you can load an iframe with a specific purpose of logging out)

[dvoytenko]

I added comments in #10647.