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 lightbox animate-in attribute removed by sanitizer #1330

Closed
p2er opened this Issue Aug 9, 2018 · 3 comments

Comments

4 participants
@p2er
Copy link

commented Aug 9, 2018

Description
Optional attribute animate-in is being removed from amp-lightbox and therefore does not allow animation configuration for lightbox even though included in the official documentation: https://www.ampproject.org/docs/reference/components/amp-lightbox#animate-in-(optional)

How to demo

  • Add lightbox to page template
  • Create page
  • Open AMP version of page
  • IS: animate-in attribute has been removed
  • SHOULD: still have animate-in attribute in page MARKUP

Example

<button on="tap:demo-lb.open">open</button>
<amp-lightbox id="demo-lb" layout="nodisplay" scrollable animate-in="fly-in-top">
<button on="tap:demo-lb.close">close</button>
<p>This lightbox should animate into viewport from top.</p>
</amp-lightbox>

@hellofromtonya hellofromtonya added this to the v1.0 milestone Aug 9, 2018

@hellofromtonya

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2018

@p2er I can replicate this issue.

We'll need to update the 'attr_spec_list for amp-lightbox in AMP_Allowed_Tags_Generated

https://github.com/Automattic/amp-wp/blob/677b7617ffb47094dab04f76e91dafb57e67ad0c/includes/sanitizers/class-amp-allowed-tags-generated.php#L2264-L2286

While in there, we should also check the attributes to the spec for amp-image-lightbox.

@westonruter

This comment has been minimized.

Copy link
Member

commented Aug 9, 2018

This will be resolved by #1315. We just need to finish it out.

@hellofromtonya hellofromtonya added this to In progress in v1.0 Aug 9, 2018

@westonruter westonruter moved this from In progress to Ready for QA in v1.0 Aug 23, 2018

@kienstra

This comment has been minimized.

Copy link
Collaborator

commented Sep 18, 2018

Moving To 'Ready For Merging'

If it's alright, I'm moving this to 'Ready For Merging,' as I don't think it needs functional testing. Feel free to move it back.

@kienstra kienstra moved this from Ready for QA to Ready for Merging in v1.0 Sep 18, 2018

@kienstra kienstra moved this from Ready for Merging to In Production in v1.0 Dec 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.