Lightbox bugfixes #14598
Lightbox bugfixes #14598
Conversation
cathyxz
commented
Apr 12, 2018
cathyxz
commented
- Bail out of transition when image height and width is 0, 0.
- Wait for carousel to be visible in close gallery to avoid race when the image shows as 0, 0.
| * @private | ||
| */ | ||
| closeGallery_() { | ||
| this.vsync_.mutate(() => { | ||
| return this.vsync_.mutatePromise(() => { |
There was a problem hiding this comment.
Should this be this.mutateElement? /cc @jridgewell
There was a problem hiding this comment.
I do need to refactor this entire component now...
| @@ -1417,11 +1424,13 @@ export class AmpLightboxGallery extends AMP.BaseElement { | |||
| } | |||
|
|
|||
| const closeGalleryAndShowTargetSlide = event => { | |||
There was a problem hiding this comment.
nit: Using class methods for listener callbacks with more than a couple lines of logic can help keep things organized
| this.closeGallery_().then(() => { | ||
| this.currentElemId_ = thumbnailObj.element.lightboxItemId; | ||
| dev().assert(this.carousel_).getImpl() | ||
| .then(carousel => carousel.showSlideWhenReady(this.currentElemId_)); |
There was a problem hiding this comment.
nit: More than one level of Promise chaining
| .implementation_.getImageBoxWithOffset(); | ||
|
|
||
| // If the image isn't initialized, bail out the transition. | ||
| if (imageBox.width == 0 && imageBox.height == 0) { |
There was a problem hiding this comment.
Hmm this seems like a proxy for something else like a lifecycle method. Is there a class method or hook you can use to know if the image viewer is initialized?
There was a problem hiding this comment.
So for this, I refactored <amp-image-viewer> to return null in the uninitialized case, so that we do a null check instead. I think doing a fade-in is better than waiting for the load, since the image could still be loading while we're animating the fade.
| element.addEventListener('click', closeGalleryAndShowTargetSlide); | ||
| const handleThumbnailClick = this.generateThumbnailClickHandler_( | ||
| thumbnailObj.element.lightboxItemId); | ||
| element.addEventListener('click', handleThumbnailClick.bind(this)); |
There was a problem hiding this comment.
It looks like you're creating a new function to save the id parameter, but that is a lot of indirection. Instead of returning a function from generateThumbnailClickHandler, use the class method to do work, then call it from the fat-arrow function and pass the id.
handleThumbnailClick_(event, id) {
event.stopPropagation();
Promise.all([
this.closeGallery_(),
dev().assert(this.carousel_).getImpl(),
]).then(values => {
this.currentElemId_ = id;
const carouselImpl = values[1];
carouselImpl.showSlideWhenReady(this.currentElemId_);
this.updateDescriptionBox_();
});
}
/* ... */
element.addEventListener('click',
event => this.handleThumbnailClick_(event, thumbnailObj.element.lightboxItemId));
| @@ -76,11 +76,11 @@ export class AmpImageViewer extends AMP.BaseElement { | |||
| /** @private {number} */ | |||
| this.sourceHeight_ = 0; | |||
|
|
|||
| /** @private {!../../../src/layout-rect.LayoutRectDef} */ | |||
| this.elementBox_ = layoutRectLtwh(0, 0, 0, 0); | |||
| /** @private {?../../../src/layout-rect.LayoutRectDef} */ | |||
There was a problem hiding this comment.
I'm now a bigger fan of being explicit about this value not being initialized as opposed to giving it a valid but incorrect (0,0,0,0) value.
|
@cvializ PTAL? =) |
* Add bail out when image is unitialized, add wait for promise to avoid race * Refactor vsync and const function * Refactor to use getImpl and measureMutate * Refactor to use measureMutate instead of vsync, change image-viewer api
