Skip to content
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

Design Review 2018-02-07 21:00 UTC (amp-img container based srcset, amp-carousel URL interface) #12940

Closed
mrjoro opened this issue Jan 21, 2018 · 4 comments

Comments

@mrjoro
Copy link
Member

mrjoro commented Jan 21, 2018

Note that there will not be a design review the week of February 12 due to AMP Conf 2018.

Time: 2018-02-07 21:00 UTC (add to Google Calendar)
Location: Video conference via Google Hangouts

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

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.

We rotate our design review between times that work better for different parts of the world as described in our design review documentation, but you are welcome to attend any design review. If you cannot make any of the design reviews but have a design to discuss please let mrjoro@ know on Slack and we will find a time that works for you.

@mrjoro mrjoro added this to the Docs Updates milestone Jan 21, 2018
@mrjoro mrjoro self-assigned this Jan 21, 2018
@aghassemi
Copy link
Contributor

aghassemi commented Jan 31, 2018

amp-img: Support both native (viewport based) and container-based srcset selection: #13178

@jamesshannon
Copy link
Contributor

amp-carousel URL interface: #12659 and #12591 are requests for URL-based slide stickiness. The original plan seems to be for a bindable [slide] attribute, but IMO it makes more sense to do something custom in order to support custom URLs and also URL updating (which could be messily done using an action like updateURLHash()).

@mrjoro
Copy link
Member Author

mrjoro commented Feb 7, 2018

amp-img container-based srcset

  • @cramforce: we probably shouldn't do this
    • e.g. when data saver is on it changes the srcset algorithm and our polyfill implementation couldn't respect that
    • what's the use case where you can't know which portion of the screen would be used by the image?
  • we don't currently support sizes, but that's a bug and should be fixed; if we did that (and we will) that should be enough
  • browsers have other optimizations (e.g. biasing towards files already on the client) that we'd potentially conflict with
  • our runtime currently does some of its own srcset logic, but we must (and we will) get rid of that; (e.g. the web packaging will need to use the native implementation)
  • we'll go with the approach of using sizes to accomplish this; we'll see if there are cases it doesn't handle
  • in the runtime you could conceivably generate a better sizes attribute based on known layout information--for stuff shown after load (like lightbox)

@mrjoro
Copy link
Member Author

mrjoro commented Feb 7, 2018

amp-carousel URL interface

  • @aghassemi: we already want to be able to change the URL based on push state rather than adding it as a custom feature for carousel (e.g. you could also add page transitions to amp-bind like in a form wizard)
    • binding the fragment to the visible slide? we don't want bind state to get applied at page load, but we could use variable substitution (currently attribute is only initially specified, not bindable)
  • @cramforce: first class support in amp-carousel is reasonable to avoid exposing the people who are interested in this feature to a more complex solution; could potentially do an amp-by-example solution
    • index and ids could be ambiguous if you have more than one carousel; ids are unique so it's the page author's problem
  • do people want a push or do they want replay? do they want to go backwards in time?
    • amp-story user studies showed that people don't want to have back button go back through the story
    • better default for a carousel is sharability without adding actual history frames; the URL fragment is what you want (it's semantically correct--you go to a different part of the page)
    • using # works better with cache because changing the URL (1.html, 2.html for the same page) the cache doesn't know those aren't actually separate pages
  • you want a good way of encoding the state in the URL that's understood by users (e.g. #san-francisco)
    • could do this via an attribute on the carousel slide, rather than just using an index
  • Justin: should amp-bind allow access to URL so you can change it?
  • timeline on history management and amp-bind? currently it's not speced for URL changes; no concrete schedule yet
  • will discuss the best solution offline

@mrjoro mrjoro closed this as completed Feb 7, 2018
@mrjoro mrjoro changed the title Design Review 2018-02-07 21:00 UTC (Americas) Design Review 2018-02-07 21:00 UTC (amp-img container based srcset, amp-carousel URL interface) Feb 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants