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

[amp-anim] add support for [amp-lightbox-gallery] #24306

Closed
maciejmackowiak opened this issue Aug 30, 2019 · 13 comments
Closed

[amp-anim] add support for [amp-lightbox-gallery] #24306

maciejmackowiak opened this issue Aug 30, 2019 · 13 comments
Labels

Comments

@maciejmackowiak
Copy link

maciejmackowiak commented Aug 30, 2019

Describe the new feature or change to an existing feature you'd like to see

I would love to use amp-lightbox-gallery when embedding gifs on page but as per AMP documentation:
obraz
gif should be embeded with amp-anim but amp-lightbox-gallery doesn't support amp-anim.
https://amp.dev/documentation/examples/components/amp-anim/

Describe alternatives you've considered

Probably I could use amp-img for gif's but:

  1. like above as per AMP documentation gif should be embedded with amp-anim.
  2. when using amp-wp this is not possible
    https://github.com/ampproject/amp-wp/blob/develop/includes/sanitizers/class-amp-img-sanitizer.php#L311
@archon810
Copy link

Curious what the solution here will end up being and whether we should implement annoying workarounds or wait for AMP to fix it. Appreciate any discussion to "feel the room" and gauge the complexity of the fix.

@archon810
Copy link

Here's an example of the issue: https://www.androidpolice.com/2019/08/28/gmail-finally-adds-that-smooth-account-switching-gesture-apk-download/?amp.

The GIFs are currently being excluded from the gallery and can't even be tapped on to zoom in.

@nainar
Copy link
Contributor

nainar commented Sep 3, 2019

Due to amp-anim's poor performance I think our intention has been to deprecate it. There is appetite for using videos to manage gifs as you can see here: #24001

Curious to hear: Is it possible to use <amp-img> for the gifs in this case? So that they are lightboxable?

@archon810
Copy link

That would certainly make things simpler for us.

@nainar
Copy link
Contributor

nainar commented Sep 4, 2019

You can currently add your gifs to <amp-img> as well. The drawback here is that <amp-img>s aren't unloaded when they leave the viewport whereas <amp-anim>s are. This means that large GIFs will stay loaded forever. If you are ok with that or are using small GIFs you can add them in <amp-img>

@archon810
Copy link

You can currently add your gifs to <amp-img> as well. The drawback here is that <amp-img>s aren't unloaded when they leave the viewport whereas <amp-anim>s are. This means that large GIFs will stay loaded forever. If you are ok with that or are using small GIFs you can add them in <amp-img>

@nainar This is the problem in AMP-WP though: https://github.com/ampproject/amp-wp/blob/develop/includes/sanitizers/class-amp-img-sanitizer.php#L311. How do we proceed?

@westonruter
Copy link
Member

@archon810 Would you create a new issue on the AMP plugin repo for this? Let's collaborate there on a way to force amp-img to be used for GIFs instead of amp-anim.

@archon810
Copy link

@maciejmackowiak will create it soon.

@westonruter
Copy link
Member

Also, here's a quick workaround plugin that forcibly converts all amp-anim to amp-img after the image sanitizer has done its conversions: https://gist.github.com/westonruter/1eeb2224004c0e904fa2a35fc92899c0

@archon810
Copy link

Fantastic - the workaround works https://www.androidpolice.com/2019/08/28/gmail-finally-adds-that-smooth-account-switching-gesture-apk-download/?amp.

We'll revert it when this ticket is eventually properly resolved.

Thanks @westonruter!

@archon810
Copy link

Any updates here please?

@stale
Copy link

stale bot commented Sep 3, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale Inactive for one year or more label Sep 3, 2021
@westonruter
Copy link
Member

westonruter commented Sep 3, 2021

I believe this will soon be obsolete with the intent to deprecate amp-img which will include, I presume, deprecation of amp-anim. See #30442. See also #24367.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants