Experiment flag for using amp-base-carousel from amp-lightbox-gallery. #21569
Conversation
- Implement methods needed for next/prev on `amp-base-carousel`. - Change carousel creation / reuse tags and selectors to use `amp-base-carousel` for the experiment.
- Fix a race between `goToSlide` and `updateUi` which would result in the call to `goToSlide` being ignored.
There was a problem hiding this comment.
We chatted offline about this: there could potentially be a race condition where if goToSlide is called in the middle of the user interacting with the carousel, it would directly jump to the requested slide and result in a jarring UX. We should either fix or file / note this behavior.
- Reword comment. - Change `updateCurrentIndex` to update `restingIndex` instead.
- Ignore programmatic scrolls when the user is scrolling. - Make multiple rapid succession calls to advance end on the correct slide.
Added logic to |
| @@ -838,6 +869,8 @@ export class Carousel { | |||
| * @private | |||
| */ | |||
| resetScrollReferencePoint_(force = false) { | |||
| this.scrolling_ = false; | |||
There was a problem hiding this comment.
Should this be after making sure that the user is not in the middle of a drag?
There was a problem hiding this comment.
I considered it, but we already have something to check for touching. So if they stopped scrolling (no scroll event recently), but they are still touching the screen, I think it would be cleaner to say that there is no scrolling, and interested code can check for touching as well. If they are dragging, we should get scroll events, so that would still count as scrolling.
There is still more work to do before actually getting this out, but setting up the groundwork.
amp-base-carousel.amp-base-carouselfor the experiment.goToSlideandupdateUiwhich would result in the callto
goToSlidebeing ignored.For #21568