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
Lightbox controls should be actually hidden and not hidden by setting opacity to 0 #13042
Conversation
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.
LGTM
@@ -934,7 +904,8 @@ export class AmpLightboxViewer extends AMP.BaseElement { | |||
this.topBar_.classList.remove('fullscreen'); | |||
} | |||
toggle(dev().assertElement(this.carousel_), true); | |||
this.toggleDescriptionBox_(true); | |||
this.updateDescriptionBox_(); |
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.
Does this.updatedesciptionBox_
need to be called when this.descriptionBox_
is toggled? It could be good defensive practice to create one method that calls both of them so no one risks forgetting to call this.updatedesciptionBox_
when toggling it.
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.
True. I'll bring that function back.
… opacity to 0 (ampproject#13042) * Lightbox controls hidden should be actually hidden and not by opacity * Refactor toggle description and update into a single function
… opacity to 0 (ampproject#13042) * Lightbox controls hidden should be actually hidden and not by opacity * Refactor toggle description and update into a single function
… opacity to 0 (ampproject#13042) * Lightbox controls hidden should be actually hidden and not by opacity * Refactor toggle description and update into a single function
Partially addresses #12993.