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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰Experiment flag to use amp-carousel 0.2 in amp-lightbox-gallery #24324

Merged
merged 5 commits into from Sep 23, 2019

Conversation

sparhami
Copy link

@sparhami sparhami commented Aug 30, 2019

We want to launch amp-carousel 0.2 for use from amp-lightbox-gallery to fix a few outstanding bugs with carousel in the lightbox. This creates an experiment flag for using the new version of carousel. Note that the version of the carousel declared on the page has preference, so if it is using 0.1, this change will have no effect.

  • Fix programmatic scroll not working after the user swiped by correctly
    clearing the actionSource flag.
  • Fix carousel selector for applying object-fit: contain, the class was
    renamed earlier.
  • Remove old logic for amp-base-carousel experiment in amp-lightbox-gallery.
  • Remove logic for stopping propagation on carousel touchmove events. The
    carousel 0.1 version of this only worked for type="carousel" and not
    type="slides". This prevents swipe to dismiss in the lightbox from working.

Fixes #24627

- Fix programmatic scroll not working after the user swiped by correctly
  clearing the `actionSource` flag.
- Fix carousel selector for applying `object-fit: contain`, the class was
  renamed earlier.
- Remove old logic for amp-base-carousel experiment in amp-lightbox-gallery.
- Remove logic for stopping propagation on carousel touchmove events. The
  carousel 0.1 version of this only worked for type="carousel" and not
  type="slides". This prevents swipe to dismiss in the lightbox from working.
@sparhami sparhami requested a review from wassgha August 30, 2019 23:56
@sparhami sparhami marked this pull request as ready for review August 30, 2019 23:56
@wassgha
Copy link
Contributor

wassgha commented Sep 9, 2019

(waiting for travis on this)

@sparhami
Copy link
Author

Fixed a few things, Travis is now passing.

@sparhami sparhami merged commit 5cf2e94 into ampproject:master Sep 23, 2019
@sparhami sparhami deleted the lb-carousel-0.2 branch September 23, 2019 18:07
@westonruter
Copy link
Member

Is this experiment still needed? Someone was puzzled seeing amp-carousel v0.1 being loaded when otherwise their site is normally using v0.2.

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

Successfully merging this pull request may close these issues.

amp-carousel 0.2 responsive image bug
4 participants