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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix bug where lightboxed carousel images show up twice #13563
Fix bug where lightboxed carousel images show up twice #13563
Conversation
19c2053
to
cd69570
Compare
ampClasses.push(cssClass); | ||
} | ||
} | ||
clonedNode.classList.remove.apply(clonedNode.classList, ampClasses); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove all classes from the clone. I don't see a lot of situations where the style for the opener should apply to the lightboxed version. If anything it can cause weird bugs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does amp-image-lightbox do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It only propagates the aria-attributes
, not the source.
Object.keys(this.ariaAttributes_).forEach(key => {
this.ariaAttributes_[key] = sourceElement.getAttribute(key);
if (this.ariaAttributes_[key]) {
this.image_.setAttribute(key, this.ariaAttributes_[key]);
}
});
@@ -221,6 +220,7 @@ export class AmpLightboxGallery extends AMP.BaseElement { | |||
const clonedNode = element.cloneNode(deepClone); | |||
clonedNode.removeAttribute('on'); | |||
clonedNode.removeAttribute('id'); | |||
clonedNode.setAttribute('class', ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removeAttribute?
9d323e4
to
6cdab96
Compare
Had to rebase for visual diff tests. |
Happens when you lightbox the image inside the carousel, like so:
The bug itself is a bit awkward. It's because I can't toggle
display: none
on an<amp-img>
with the.amp-scrollable-carousel-slide
class, since it sets thedisplay
css property with!important
. I'm not sure whether we should be using!important
there, and I'm also not sure if theamp-scrollable-carousel-slide
class should be prefixed withi-amphtml
instead ofamp-
.It seems reasonable to strip amp-related css classes, since basically what we're doing is taking the raw image with its raw aspect ratio, and blowing it to fill the screen. I'm wondering if we could just strip ALL CSS classes, or are there situations where it would be valid to keep the user-defined css classes? It's possible, like maybe they apply an opacity, or other visual effects...