Don't show nav or gallery controls if there is only one lightboxed image #15475
Conversation
cathyxz
force-pushed
the
feature/lightbox-one-item
branch
2 times, most recently
from
4449dff
to
ddf80c2
Compare
|
Rebasing for bundle size flake. |
cathyxz
force-pushed
the
feature/lightbox-one-item
branch
from
ddf80c2
to
748ea8d
Compare
|
This pull request introduces 1 alert when merging 748ea8d into d175fb8 - view on lgtm.com new alerts:
Comment posted by lgtm.com |
cathyxz
force-pushed
the
feature/lightbox-one-item
branch
from
748ea8d
to
fe69ae0
Compare
| */ | ||
| toggleNavControls_(noOfChildren) { | ||
| if (noOfChildren > 1) { | ||
| this.controlsContainer_.classList.remove('i-amphtml-lbg-single'); |
There was a problem hiding this comment.
nit: You can use classList.toggle e.g.
this.controlsContainer_.classList.toggle('i-amphtml-lbg-single', noOfChildren <= 1);There was a problem hiding this comment.
super optional nit: childCount is easier to read since it doesn't use an abbreviation
There was a problem hiding this comment.
Do you plan to use the CSS class i-amphtml-lbg-single for other purposes? If not, you can toggle the internal class i-amphtml-hidden that AMP already uses to hide elements.
There was a problem hiding this comment.
Oh... hmm. I'll need to toggle it for three elements I guess.
There was a problem hiding this comment.
That's true, that could introduce complexity. I'm fine with the CSS how it is.
There was a problem hiding this comment.
FYI I just checked though. We don't actually have an i-amphtml-hidden css class. We have an amp-hidden class. It doesn't have default behavior, but each component defines behavior for it separately...
Closes #15198, since
<amp-lightbox-gallery>is now launched, and this covers the single-image usecase.