Implement lightbox attribute for carousels #12771

cathyxz · 2018-01-10T21:07:19Z

Partially addresses #12583. Enables lightboxing carousels and installs on-tap handlers.
Basically implements a special case if the tag name is amp-carousel to lightbox all its children instead. Finding the children has different logic depending on whether the type of the amp-carousel is carousel or slides.

Known Bugs:

There's a known issue with rendering the third image onwards for carousels with type='slides'. This is due to us initializing the image viewer on buildCarousel, and expecting the amp-img to be laid out when it isn't. The intended fix is to promote ImageViewer to its own standalone component and to call its layout callback when we are one slide away and ready to lay out that slide in the lightbox.

aghassemi

awesome. couple of requests.

aghassemi · 2018-01-10T21:52:53Z

extensions/amp-lightbox-viewer/0.1/amp-lightbox-viewer.js

    this.resources_ = Services.resourcesForDoc(this.getAmpDoc());
-    this.buildMask_();
-    this.element.appendChild(this.container_);
+    this.vsync_.mutate(() => {


on second thought, viewer.whenFirstVisible().then is probably better than a mutate. Difference is that buildCallback is called during prerender mode. Initializing a lightbox is too much work IMO for prerender mode. viewer.whenFirstVisible(). will ensure initializing code is delayed until page is actually shown.

aghassemi · 2018-01-10T22:01:32Z

extensions/amp-lightbox-viewer/0.1/service/lightbox-manager-impl.js

+   * @private
+   */
+  getSlidesFromCarousel_(element) {
+    const type = element.getAttribute('type');


I think for both types of carousels, each slide has amp-carousel-slide class attached to it. So this could become

toArray(element.getElementsByClassname('amp-carousel-slide');

I think there is also a race-condition here. There is no gaurantee that carousel has finish building by the time it is discovered. so we would need to wait for the "LOAD_END" signal of carousel before querying it. Something like element.whenSignal(CommonSignals.LOAD_END).then should do.

aghassemi · 2018-01-10T22:09:06Z

extensions/amp-lightbox-viewer/0.1/service/lightbox-manager-impl.js

+    let carouselSlides = [];
+    if (type == 'slides') {
+      iterateCursor(
+          scopedQuerySelectorAll(element, SLIDE_ITEM_SELECTOR),


scopedQuerySelectorAll is usually only needed when the query itself has space or > to query descendants. For id, class or attribute selectors, normalyl querySelector is fine

This was intentional because there might be multiple carousels in the page, so you only want to select the slides of that one particular carousel.

cvializ · 2018-01-10T22:03:50Z

extensions/amp-lightbox-viewer/0.1/service/lightbox-manager-impl.js

+   * @private
+   */
+  getSlidesFromCarousel_(element) {
+    const type = element.getAttribute('type');


I'm not sure this approach is best. Maybe we could add a new public method getSlides to the base-carousel that slidescroll and scrollable-carousel could each implement to expose their slide elements. That way you don't need to have the branches here, and if the carousels ever update how they contain their slides, the implementer won't need to be aware of this code here.

I like this idea a lot actually. Seems a lot better than my current hack.

cvializ · 2018-01-10T22:08:30Z

extensions/amp-lightbox-viewer/0.1/service/lightbox-manager-impl.js

+      const container = dev().assertElement(
+          scopedQuerySelector(element, CAROUSEL_CONTAINER_SELECTOR)
+      );
+      carouselSlides = childElements(container, () => true);


You could use the Element.children property instead.

cvializ · 2018-01-10T22:11:26Z

extensions/amp-lightbox-viewer/0.1/service/lightbox-manager-impl.js

+    if (type == 'slides') {
+      iterateCursor(
+          scopedQuerySelectorAll(element, SLIDE_ITEM_SELECTOR),
+          slide => carouselSlides.push(childElement(slide, () => true))


The () => true looks fishy. You should use Element.firstElementChild instead

cvializ · 2018-01-10T22:12:50Z

extensions/amp-lightbox-viewer/0.1/service/lightbox-manager-impl.js

+
+    /**
+     * Counter tracking number of carousels without ids
+     * @private {!number}


nit: primitives are non nullable by default

I truly thought I had rid all these files of this thing... welp. Tyty.

np, honestly we should have a tool that shouts at us for it. I'm surprised closure doesn't by default

cvializ · 2018-01-10T22:14:57Z

extensions/amp-lightbox-viewer/0.1/service/lightbox-manager-impl.js

-    if (element.tagName == 'AMP-CAROUSEL') {
-      // TODO: special case carousel
+    if (element.tagName.toLowerCase() == CAROUSEL_TAG) {
+      let lightboxGroupId = element.getAttribute('lightbox');


This should be simplified to const lightboxGroupId = element.getAttribute('lightbox') || ...

Does this still read well?

const lightboxGroupId = element.getAttribute('lightbox') || 'carousel' + (element.getAttribute('id') || this.counter_++);

imho it's more readable since the const and || communicates a fallback value better than a let and an if statement. But if you feel like it's more cluttered this way, follow your own style-compass : )

cvializ · 2018-01-10T22:15:38Z

extensions/amp-lightbox-viewer/0.1/service/lightbox-manager-impl.js

+        .push(dev().assertElement(element));
+    if (this.meetsHeuristicsForTap_(element)) {
+      const viewer = elementByTag(this.ampdoc_.getRootNode(), VIEWER_TAG);
+      element.setAttribute('on', 'tap:' + viewer.id + '.activate');


Feel free to use es6 template strings for simple string interpolation like this.

cvializ · 2018-01-10T22:21:32Z

extensions/amp-lightbox-viewer/0.1/amp-lightbox-viewer.js

+      this.container_ = this.win.document.createElement('div');
+      this.container_.classList.add('i-amphtml-lbv');
+      this.element.appendChild(this.container_);
+      this.manager_.maybeInit();


Why do you need to call maybeInit here now? Does it not need to be lazily initialized any more?

It cannot be lazy initialized on open anymore because it handles adding on tap handlers to everything that has the lightbox attribute. open is usually triggered on tap.

aghassemi · 2018-01-11T18:04:48Z

extensions/amp-lightbox-viewer/0.1/service/lightbox-manager-impl.js

+   */
+  getSlidesFromCarousel_(element) {
+    return element.signals().whenSignal(CommonSignals.LOAD_END).then(() => {
+      return toArray(scopedQuerySelectorAll(element, SLIDE_SELECTOR));


This is fine but note that element.querySelectorAll('.someClassName) will still only return someClassName descendants of element.

The whole thing is pretty confusing and something I learned recently. When querySelector goes wrong section of this article https://developers.google.com/web/updates/2013/03/What-s-the-CSS-scope-pseudo-class-for has a good explanation of what :scope pseudo-selector (which our scopedQuerySelectorAll is based on) is and what issues it fixes, it is worth a quick read.

Yes, I think the intention is to return someClassName descendants of element. The element here is the carousel that I'm lightboxing. I'm looking for the slides of that carousel only, which are necessarily descendants of the lightbox. If I don't scope this selector, I might accidentally get slides of other carousels in the document. So I think this implementation is consistent with the intention?

Maybe what's confusing here is the naming of the input parameter. It's currently called element, but should really be carousel.

I think querySelector returns only one element, whereas querySelectorAll returns all elements that match the selector. We do want querySelectorAll. As to why not use element.querySelectorAll, I think that's precisely due to the issue in the article. I think the usage of scopedQuerySelectorAll here is the correct one (the other ones also don't lint =P ).

Oh I see. There's a performance hit. Basically element.querySelectorAll will still work, but won't have the perf hit associated with scopedQuerySelectorAll. Since this use case isn't necessary for scopedQuerySelectorAll, it's better not to have the perf it and just to /*OK*/ lint. Is that what you mean @aghassemi ?

correct. The reason I brought it up wasn't necessarily for the perf gains (which are not much if any), but mostly to tell what I learned regarding scropeSelector recently which was unintuitive to me so though it is worth sharing.

Alss note that the reasons our presubmit complains about element.querySelectorAll(VARIABLE) is that a variable is passed to it so it can not make a the right judgment whether the query is one of the problematic ones or not. If a string literal was passed to it, it would not have complained.

cathyxz · 2018-01-11T21:32:20Z

@cvializ PTAL? Just one unresolved discussion here about element.querySelectorAll and scopedQuerySelectorAll. =)

aghassemi · 2018-01-11T21:46:16Z

comments all addressed

* Draft work on getting carousels to work with the lightbox attribute * Address comments * Use signals for detecting carousel load in amp-lightbox-viewer as well * Change scopedQuerySelectorAll to querySelectorAll for perf benefit

Draft work on getting carousels to work with the lightbox attribute

8cc93b4

googlebot added the cla: yes label Jan 10, 2018

cathyxz requested review from cvializ and aghassemi January 10, 2018 21:23

aghassemi suggested changes Jan 10, 2018

View reviewed changes

cvializ previously requested changes Jan 10, 2018

View reviewed changes

cathyxz added 2 commits January 10, 2018 16:02

Address comments

9b57f05

Use signals for detecting carousel load in amp-lightbox-viewer as well

62bcc6c

aghassemi approved these changes Jan 11, 2018

View reviewed changes

Change scopedQuerySelectorAll to querySelectorAll for perf benefit

1c70063

cathyxz assigned cvializ Jan 11, 2018

cathyxz merged commit fe4b18c into ampproject:master Jan 11, 2018

cathyxz deleted the feature/carousel-lightbox branch January 11, 2018 22:32

cathyxz mentioned this pull request Jan 19, 2018

lightbox attribute for amp-carousel #11660

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement lightbox attribute for carousels #12771

Implement lightbox attribute for carousels #12771

cathyxz commented Jan 10, 2018 •

edited

aghassemi left a comment

aghassemi Jan 10, 2018

aghassemi Jan 10, 2018

aghassemi Jan 10, 2018

cathyxz Jan 10, 2018

cvializ Jan 10, 2018

cathyxz Jan 10, 2018

cvializ Jan 10, 2018

cvializ Jan 10, 2018

cvializ Jan 10, 2018

cathyxz Jan 10, 2018

cvializ Jan 11, 2018

cvializ Jan 10, 2018

cathyxz Jan 10, 2018

cvializ Jan 10, 2018

cvializ Jan 10, 2018

cvializ Jan 10, 2018

cathyxz Jan 10, 2018 •

edited

aghassemi Jan 11, 2018

cathyxz Jan 11, 2018

cathyxz Jan 11, 2018

cathyxz Jan 11, 2018

aghassemi Jan 11, 2018

cathyxz commented Jan 11, 2018

aghassemi commented Jan 11, 2018

Implement lightbox attribute for carousels #12771

Implement lightbox attribute for carousels #12771

Conversation

cathyxz commented Jan 10, 2018 • edited

aghassemi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cathyxz Jan 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cathyxz commented Jan 11, 2018

aghassemi commented Jan 11, 2018

cathyxz commented Jan 10, 2018 •

edited

cathyxz Jan 10, 2018 •

edited