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

Allow the amp-carousel script to be used on page when there is just amp-lightbox-gallery #6509

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Aug 3, 2021

Summary

Allow the amp-carousel script as a special case to be on the page when there is no since the
amp-lightbox-gallery component will lazy-load the amp-carousel script when a lightbox is opened, and since
amp-carousel v0.1 is still the 'latest' version, this can mean that fixes needed with the 0.2 version won't
be present on the page. Adding the amp-carousel v0.2 script is a stated workaround suggested in an AMP core
issue: ampproject/amphtml#35402 (comment).

See also #3115 (comment).

Checklist

  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@westonruter westonruter force-pushed the update/allow-amp-carousel-when-only-amp-lightbox-gallery-present branch from a2008ae to 972054a Compare August 3, 2021 20:02
@westonruter westonruter marked this pull request as ready for review August 3, 2021 20:02
@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2021

Plugin builds for 972054a are ready 🛎️!

Copy link
Contributor

@pierlon pierlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :shipit:

@westonruter westonruter merged commit 22b3ccb into develop Aug 3, 2021
@westonruter westonruter deleted the update/allow-amp-carousel-when-only-amp-lightbox-gallery-present branch August 3, 2021 20:22
westonruter added a commit that referenced this pull request Aug 3, 2021
westonruter added a commit that referenced this pull request Aug 3, 2021
…image-dimension

* 'develop' of github.com:ampproject/amp-wp:
  Allow the amp-carousel script to be on the page when there is just amp-lightbox-gallery (#6509)
  Mock HTTP request
  Supply non-empty body and headers for tests
  Only unserialize cached response if it is a string
  Update amp-twitter to use fixed-height layout with a default height matching minimal tweet height
  Bump eslint from 7.31.0 to 7.32.0
  Use OVERFLOW constant
  Remove unused `use` statements
  Add overflow buttons to Twitter, Facebook, and Instagram embeds
  Pin amp-toolbox-php to specific commit in main
  Update amp-toolbox-php to 98b020d6
  Remove unused AmpProject\Dom\Document imports
  Update references to renamed class constants and remove old deprecated code
  Update amp-toolbox-php to 0.7.0-alpha (main@a155c37)
  Update snapshot
  Use unique IDs for each SVG component instantiation
  Avoid redirect_extraneous_paired_endpoint from needlessly redirecting
  Revert "Prevent mutating URL when query var is absent"
  Prevent mutating URL when query var is absent
pierlon pushed a commit to pierlon/amp-wp that referenced this pull request Aug 15, 2021
@westonruter westonruter self-assigned this Aug 30, 2021
@westonruter
Copy link
Member Author

QA Passed

When adding the following plugin code:

add_action( 'wp_enqueue_scripts', function () {
	wp_enqueue_script('amp-carousel');
} );

The amp-carousel v0.2 script is added to the page when image has the lightbox effect, thus preventing the auto-loading of v0.1 of amp-carousel.

If no image has the lightbox effect, then the amp-carousel script is omitted.

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

Successfully merging this pull request may close these issues.

2 participants