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.

Sign up for GitHub

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

Jump to bottom

Sync Lightbox with Carousel #13050

Merged
merged 5 commits into from Jan 26, 2018
Merged

Sync Lightbox with Carousel #13050

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
708aeb9
Sync slides to lightbox
cathyxz Jan 26, 2018
8524ec3
Only exit if you're going back to your original element or you're a s…
cathyxz Jan 26, 2018
e700c75
Exclude scroll carousel
cathyxz Jan 26, 2018
2e31805
Add null check to shouldAnimate
cathyxz Jan 26, 2018
d6daf5d
Handle excluding elements from lightbox
cathyxz Jan 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
71 changes: 48 additions & 23 deletions extensions/amp-lightbox-viewer/0.1/amp-lightbox-viewer.js
View file
Open in desktop
Expand Up @@ -74,7 +74,8 @@ let manager_;
* @typedef {{
* descriptionText: string,
* tagName: string,
* imageViewer: ?Element
* imageViewer: ?Element,
* sourceElement: !Element
* }}
*/
let LightboxElementMetadataDef_;
Expand Down Expand Up @@ -234,6 +235,7 @@ export class AmpLightboxViewer extends AMP.BaseElement {
const metadata = {
descriptionText: descText,
tagName: clonedNode.tagName,
sourceElement: element,
};
let slide = clonedNode;
if (clonedNode.tagName === 'AMP-IMG') {
Expand Down Expand Up @@ -306,6 +308,12 @@ export class AmpLightboxViewer extends AMP.BaseElement {
slideChangeHandler_(event) {
this.currentElemId_ = getData(event)['index'];
this.updateDescriptionBox_();
const sourceCarousel = this.manager_
.getCarouselForLightboxGroup(this.currentLightboxGroupId_);
if (sourceCarousel) {
/**@type {?}*/ (sourceCarousel).implementation_
.showSlideWhenReady(this.currentElemId_);
}
}

/**
Expand Down Expand Up @@ -676,17 +684,25 @@ export class AmpLightboxViewer extends AMP.BaseElement {
}

/**
* @param {!Element} ampImage
* This function verifies that the source element is an amp-img and contains
* an img element and preserves the natural aspect ratio of the original img.
* @param {!Element} element
* @return {boolean}
* @private
*/
aspectRatioChanged_(ampImage) {
const img = elementByTag(dev().assertElement(ampImage), 'img');
shouldAnimate_(element) {
if (element.tagName !== 'AMP-IMG') {
return false;
}
const img = elementByTag(dev().assertElement(element), 'img');
if (!img) {
return false;
}
const naturalAspectRatio = img.naturalWidth / img.naturalHeight;
const elementHeight = ampImage./*OK*/offsetHeight;
const elementWidth = ampImage./*OK*/offsetWidth;
const elementHeight = element./*OK*/offsetHeight;
const elementWidth = element./*OK*/offsetWidth;
const ampImageAspectRatio = elementWidth / elementHeight;
return Math.abs(naturalAspectRatio - ampImageAspectRatio) > EPSILON;
return Math.abs(naturalAspectRatio - ampImageAspectRatio) < EPSILON;
}

/**
Expand All @@ -699,28 +715,28 @@ export class AmpLightboxViewer extends AMP.BaseElement {
const anim = new Animation(this.element);
let duration = MIN_TRANSITION_DURATION;
let transLayer = null;
const sourceElement = this.getCurrentElement_().sourceElement;
return this.vsync_.measurePromise(() => {
// Lightbox background fades in.
anim.add(0, tr.setStyles(this.element, {
opacity: tr.numeric(0, 1),
}), MOTION_DURATION_RATIO, ENTER_CURVE_);

// Try to transition from the source image.
if (this.sourceElement_ && isLoaded(this.sourceElement_)
&& !this.aspectRatioChanged_(this.sourceElement_)) {
if (sourceElement && isLoaded(sourceElement)
&& this.shouldAnimate_(sourceElement)) {

// TODO (#13039): implement crop and object fit contain transitions
transLayer = this.element.ownerDocument.createElement('div');
transLayer.classList.add('i-amphtml-lightbox-viewer-trans');
this.element.ownerDocument.body.appendChild(transLayer);
const rect = layoutRectFromDomRect(this.sourceElement_
const rect = layoutRectFromDomRect(sourceElement
./*OK*/getBoundingClientRect());

const imageBox = /**@type {?}*/ (this.getCurrentElement_().imageViewer)
.implementation_.getImageBoxWithOffset();

const clone = this.sourceElement_.cloneNode(true);

const clone = sourceElement.cloneNode(true);
clone.className = '';
st.setStyles(clone, {
position: 'absolute',
Expand All @@ -733,8 +749,6 @@ export class AmpLightboxViewer extends AMP.BaseElement {
});
transLayer.appendChild(clone);

this.sourceElement_.classList.add('i-amphtml-ghost');

// Move and resize the image to the location given by the lightbox.
const dx = imageBox.left - rect.left;
const dy = imageBox.top - rect.top;
Expand Down Expand Up @@ -783,21 +797,32 @@ export class AmpLightboxViewer extends AMP.BaseElement {
exit_() {
const anim = new Animation(this.element);
let duration = MIN_TRANSITION_DURATION;
const imageBox = /**@type {?}*/ (this.getCurrentElement_().imageViewer)
const currentElementMetadata = this.getCurrentElement_();
const imageBox = /**@type {?}*/ (currentElementMetadata.imageViewer)
.implementation_.getImageBoxWithOffset();
const image = /**@type {?}*/ (this.getCurrentElement_().imageViewer)
const image = /**@type {?}*/ (currentElementMetadata.imageViewer)
.implementation_.getImage();
const sourceElement = currentElementMetadata.sourceElement;
// Try to transition to the source image.
let transLayer = null;

return this.vsync_.measurePromise(() => {
if (this.sourceElement_ && image
&& !this.aspectRatioChanged_(this.sourceElement_)) {
// TODO (#13013): if current image is not the original image, don't transition
// Lightbox background fades out.
anim.add(0, tr.setStyles(this.element, {
opacity: tr.numeric(1, 0),
}), MOTION_DURATION_RATIO, ENTER_CURVE_);

if (sourceElement !== null
&& sourceElement.tagName == 'AMP-IMG'
&& this.shouldAnimate_(sourceElement)
&& (sourceElement == this.sourceElement_
|| this.manager_.hasCarousel(this.currentLightboxGroupId_))) {
transLayer = this.element.ownerDocument.createElement('div');
transLayer.classList.add('i-amphtml-lightbox-viewer-trans');
this.element.ownerDocument.body.appendChild(transLayer);
sourceElement.classList.add('i-amphtml-ghost');

const rect = layoutRectFromDomRect(this.sourceElement_
const rect = layoutRectFromDomRect(sourceElement
./*OK*/getBoundingClientRect());
const clone = image.cloneNode(true);
st.setStyles(clone, {
Expand Down Expand Up @@ -835,7 +860,7 @@ export class AmpLightboxViewer extends AMP.BaseElement {
anim.add(0, (time, complete) => {
moveAndScale(time);
if (complete) {
this.sourceElement_.classList.remove('i-amphtml-ghost');
sourceElement.classList.remove('i-amphtml-ghost');
}
}, MOTION_DURATION_RATIO, EXIT_CURVE_);

Expand All @@ -849,8 +874,8 @@ export class AmpLightboxViewer extends AMP.BaseElement {
}).then(() => {
return anim.start(duration).thenAlways(() => {
return this.vsync_.mutatePromise(() => {
if (this.sourceElement_) {
this.sourceElement_.classList.remove('i-amphtml-ghost');
if (sourceElement) {
sourceElement.classList.remove('i-amphtml-ghost');
}
st.setStyles(this.element, {
opacity: '',
Expand Down
34 changes: 34 additions & 0 deletions extensions/amp-lightbox-viewer/0.1/service/lightbox-manager-impl.js
View file
Open in desktop
Expand Up @@ -89,6 +89,13 @@ export class LightboxManager {
* @private {number}
*/
this.counter_ = 0;

/**
* If the lightbox group is a carousel, this object contains a
* mapping of the lightbox group id to the carousel element.
* @private {!Object<string, !Element>}
*/
this.lightboxSourceCarousels_ = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

This might need to be a src/object.js#map to speed up lookups. map is prototypeless so failed lookups happen faster. In the AMP codebase I see we generally prefer maps and map[key] notation vs {} literals and hasOwnProperty.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jridgewell any guidance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed this and yes you're right. This should happen for security reasons too, not just performance.

}

/**
Expand All @@ -103,6 +110,28 @@ export class LightboxManager {
return this.initPromise_;
}

/**
* Returns a reference to the source carousel of the lightbox
* group if one exists.
* @param {string} lightboxGroupId
* @return {Element|null}
*/
getCarouselForLightboxGroup(lightboxGroupId) {
if (this.lightboxSourceCarousels_.hasOwnProperty(lightboxGroupId)) {
return this.lightboxSourceCarousels_[lightboxGroupId];
}
return null;
}

/**
* Returns true if the lightboxGroupId belongs to an amp carousel
* @param {string} lightboxGroupId
* @return {boolean}
*/
hasCarousel(lightboxGroupId) {
return this.lightboxSourceCarousels_.hasOwnProperty(lightboxGroupId);
}

/**
* Decides whether an already lightboxable element should automatically get
* a tap handler to open in the lightbox.
Expand Down Expand Up @@ -155,6 +184,11 @@ export class LightboxManager {
processLightboxCarousel_(carousel) {
const lightboxGroupId = carousel.getAttribute('lightbox') ||
'carousel' + (carousel.getAttribute('id') || this.counter_++);
if (carousel.getAttribute('type') == 'slides') {
this.lightboxSourceCarousels_[lightboxGroupId] = carousel;
// TODO (#13011): scroll carousel needs to support goToSlide
// before we can use it for lightbox, so they currently don't count.
}
this.getSlidesFromCarousel_(carousel).then(slides => {
slides.forEach(slide => {
if (!slide.hasAttribute('lightbox-exclude')) {
Expand Down