✨auto-lightbox carousels under experiment #20910
Conversation
| return resolveFalse(); | ||
| } | ||
|
|
||
| const slides = element.querySelectorAll('.amp-carousel-slide'); |
There was a problem hiding this comment.
there is a race condition since it carousel extension is not loaded yet, we don't have this class.
There was a problem hiding this comment.
Nope, this runs after layoutCallback.
| this.getSlidesFromCarousel_(carousel).then(slides => { | ||
| slides.forEach(slide => { | ||
| const shouldExcludeSlide = slide.hasAttribute('lightbox-exclude') | ||
| || (slide.hasAttribute('lightbox') | ||
| const shouldExcludeSlide = |
There was a problem hiding this comment.
This boolean assignment is getting complex. Maybe split a subexpression into another named boolean?
const hasExistingLightbox = (slide.hasAttribute('lightbox') &&
slide.getAttribute('lightbox') !== lightboxGroupId);
const shouldExcludeSlide = (slide.hasAttribute('lightbox-exclude') || hasExistingLightbox);There was a problem hiding this comment.
This assignment is exactly the same as before, just indented differently.
| } | ||
| element = unwrappedFigureElement; |
| { | ||
| id: 'amp-auto-lightbox-carousel', | ||
| name: 'Automatically detects carousels to group in a lightbox.', | ||
| spec: 'https://github.com/ampproject/amphtml/issues/20395', |
There was a problem hiding this comment.
sanity check: these github issues are the same as the above. possible copypaste error or intentional?
There was a problem hiding this comment.
Intentional, not really necessary to divide the spec in Github according to our rollout.
| static meetsSizingCriteria(img, slide) { | ||
| devAssert(img.tagName == 'AMP-IMG'); | ||
|
|
||
| return img.getImpl().then(impl => new Promise(resolve => { |
There was a problem hiding this comment.
I believe measureElement returns a Promise that contains the return value of the measurer function, so this new wrapper Promise is redundant. You can do
return img.getImpl().then(impl => {
return impl.measureElement(() => {
/*...*/
return (imageArea/slideArea >= MIN_IMG_SLIDE_AREA_RATIO);
});
});There was a problem hiding this comment.
Unfortunately the measure promise is shared by the entire batch so returning values is not possible.
There was a problem hiding this comment.
@aghassemi @cvializ Simplified per offline discussion and comments here. PTAL.
| { | ||
| id: 'amp-auto-lightbox-carousel', | ||
| name: 'Automatically detects carousels to group in a lightbox.', | ||
| spec: 'https://github.com/ampproject/amphtml/issues/20395', |
There was a problem hiding this comment.
Intentional, not really necessary to divide the spec in Github according to our rollout.
| this.getSlidesFromCarousel_(carousel).then(slides => { | ||
| slides.forEach(slide => { | ||
| const shouldExcludeSlide = slide.hasAttribute('lightbox-exclude') | ||
| || (slide.hasAttribute('lightbox') | ||
| const shouldExcludeSlide = |
There was a problem hiding this comment.
This assignment is exactly the same as before, just indented differently.
| return resolveFalse(); | ||
| } | ||
|
|
||
| const slides = element.querySelectorAll('.amp-carousel-slide'); |
There was a problem hiding this comment.
Nope, this runs after layoutCallback.
| static meetsSizingCriteria(img, slide) { | ||
| devAssert(img.tagName == 'AMP-IMG'); | ||
|
|
||
| return img.getImpl().then(impl => new Promise(resolve => { |
There was a problem hiding this comment.
Unfortunately the measure promise is shared by the entire batch so returning values is not possible.
* Introduces criteria to accept any carousel that:
1. Has one image on every slide
2. Has no valid `on=tap` actions.
* Since `amp-img` has to be measured against the slide element, some APIs are now async.
* Introduces a `mixed` manual test for several cases.
* Introduces criteria to accept any carousel that:
1. Has one image on every slide
2. Has no valid `on=tap` actions.
* Since `amp-img` has to be measured against the slide element, some APIs are now async.
* Introduces a `mixed` manual test for several cases.
on=tapactions.amp-imghas to be measured against the slide element, some APIs are now async.mixedmanual test for several cases.