🚀 [Story interactive] Rewrite Image URL to Cached URL for Image Quizzes and Polls #35375
Conversation
|
Hey @gmajoulet! These files were changed: Hey @mszylkowski! These files were changed: Hey @newmuis! These files were changed: |
There was a problem hiding this comment.
Let's not use a specific function that sets up the images, I think it's cleaner to keep the image setup inside the attachContent function as before. Also, we can move the call to maybeMakeProxyUrl to the parseOptions for the image attr so any images have this fix. Then we can get @gmajoulet review on this since he has some more context
| url, | ||
| urlService.getSourceOrigin(loc.href) | ||
| ); | ||
| return loc.origin + '/i/s/' + resolvedRelativeUrl.replace(/https?:\/\//, ''); |
There was a problem hiding this comment.
These URLs support a bunch of useful parameters that we should add to the URL, like max width etc. Please check the "URL path" documentation here: https://amp.dev/documentation/guides-and-tutorials/learn/amp-caches-and-cors/amp-cache-urls/
I think specifying a width here would be useful and save some bytes
There was a problem hiding this comment.
Dynamic resize is expensive, and sacrifices AVIF encoding (because that's too expensive to do on the fly). Using a regular image URL will likely be faster due to better caching, faster response times, and better encoding formats. Using an oversized image is a publisher fault.
There was a problem hiding this comment.
Definitely using oversized images are not great for components that show the images in small regions. When passing these parameters do the images get resized every time, or would they only be resized on the first request to that resource at that resolution? I think for these types of images we'd only want to ask for the small image if we can, and not the original sized asset
There was a problem hiding this comment.
When passing these parameters do the images get resized every time, or would they only be resized on the first request to that resource at that resolution?
They're resized every time. The persistence layer never stores resizes, only originals. The local-cache may keep an resized item, but it's an extremely small LRU competing with literal billions of other responses.
I think for these types of images we'd only want to ask for the small image if we can, and not the original sized asset
What size is the original asset? Is the publisher aware this image is being shown in a small area?
There was a problem hiding this comment.
Thank you for chiming in Justin, let's keep /i/s and no resizing then.
extensions/amp-story/1.0/utils.js
Outdated
| export const maybeMakeProxyUrl = (url, pageEl) => { | ||
| const urlService = Services.urlForDoc(pageEl); | ||
| const loc = pageEl.getAmpDoc().win.location; |
There was a problem hiding this comment.
Now that it's moved to utils.js I think we should make it easier to re-use, and accept url, ampdoc as parameters.
urlForDoc(ampdoc) should work, and both classes using this helper should be able to call this.getAmpDoc().
extensions/amp-story-interactive/0.1/amp-story-interactive-abstract.js
Outdated
Show resolved
Hide resolved
| @@ -44,6 +45,7 @@ describes.realWin( | |||
| const ampStoryPollEl = win.document.createElement( | |||
| 'amp-story-interactive-binary-poll' | |||
| ); | |||
| ampStoryPollEl.getAmpDoc = () => new AmpDocSingle(win); | |||
There was a problem hiding this comment.
If we use this.getAmpDoc(), we might not need to mock this (or at least mock it directly on the AmpStoryInteractiveBinaryPoll instead of it's element)
…tok-validation * 'main' of github.com:ampproject/amphtml: (72 commits) build: run amp lint --fix to address import order of jixie (ampproject#35513) ✨ [amp-analytics] Add Custom Browser Event Tracker (ampproject#35193) babel: teach amp mode transformer about #core/mode (ampproject#35477) 🚮 Remove experiment `amp-consent-granular-consent` (ampproject#35508) ♻️ Enable auto-sorting+grouping within src/ and 3p/ (ampproject#35454) 🐛 [amp-render] fix root-element stripping from amp-render with amp-bind (ampproject#35449) ✅ [Story interactive] Add Example Story for Detailed Results Component (ampproject#35450) 🐛 Fix error on bento example (ampproject#35490) 🐛 amp-story-grid-layer: Fix AMP invalidation error in documentation (ampproject#35503) 🐛 Fix code formatting (ampproject#35499) ✅ buildDom: add tests for amp-fit-text and amp-layout (ampproject#35494) ♻️ Remove unused imports (ampproject#35435) ✨ amp-connatix-player: iframe domain based on a property (ampproject#35179) Updated document with use cases of remote config (ampproject#35496) AMP.goBack: update documentation (ampproject#29290) 🏗 Allow the bundle-size job to run even if the builds were skipped (ampproject#35492) build-system: improve terser/esbuild integration (ampproject#35466) 🧪 [Story performance] Disable animations on first page to 50% (ampproject#35476) 📖 [Amp story] [Page attachments] Amp.dev Docs for New Page Attachment Features ampproject#34883 (ampproject#35338) 🚀 [Story interactive] Rewrite Image URL to Cached URL for Image Quizzes and Polls (ampproject#35375) ...
Rewrite image URLs of image quizzes and polls on the AMP cache to use the cached URLs
Closes #35109
/cc @mszylkowski
/cc @processprocess