✨ Bento Lightbox Gallery

[caroqliu]

Current approach

<LightboxGallery>
  <WithLightbox>
    <img />            // Lightboxed
  </WithLightbox>
  <img />                     // Not lightboxed
  <WithLightbox render={() => <img />}>
    <video />            // Lightboxed with thumbnail
  </WithLightbox>
  <p>Hello world</p>          // Not lightboxed
  <BaseCarousel lightbox>     // All children are lightboxed, respecting any with `thumbnailSrc`
    <img />
    <video thumbnailSrc="img.png"/>
    <div> slide 3 </div>
  </BaseCarousel>
</LightboxGallery>

Original approach (Stale)

<LightboxGallery>
  <img lightbox />            // Lightboxed
  <img />                     // Not lightboxed
  <p>Hello world</p>          // Not lightboxed
  <BaseCarousel lightbox>     // All img children are lightboxed
    <img />
    <video />
    <div> slide 3 </div>
  </BaseCarousel>
</LightboxGallery>

This is a significant change from 0.1 API, which has the following two foundational features:

• lightbox attribute is all that is needed to place all such labelled img and carousel elements
• all lightboxed elements share a single lightbox UI for the full document

One way to use this in AMP and conform the API is to wrap all documents with <amp-lightbox-gallery>, but this is not ideal as it would disrupt user supplied document structure. At the Preact layer, it's not obvious that a document-wide service is the way to go, so it's possible the composability and specificity of use could be a benefit for that environment.

Alternative approaches:

• Define useLightbox hook in AmpContext, but only have it work if the user imports LightboxGallery, and not be an undo burden if they do not. Then the carousel components would support a prop lightbox which would use useLightbox. This would not work however for native img elements.

Partial for #32759

[dvoytenko]

Ok, I think I understood your process here. And it makes sense to me. One nuance - it cannot traverse nested children. But we could enable it with making WithLightbox public. E.g. the user code could look like this:

Somewhere once:

<LightboxGallery>  // could also call it `(Auto)LightboxProvider`
  {children}
</LightboxGallery>

And elsewhere:

function SomeComponent() {
  return (
    <div>
      <WithLightbox>
         <img ..../>
      </WithLightbox>
    </div>
  );
}

This might be pretty clean from Preact point of view. Some of our components could even call WithLightbox internally.

In AMP we could enable this somehow too:

1. The LightboxProvider attachment is very easy for us now - all it needs to do is to propagate context. We do this today with InlineGallery.
2. The WithLightbox would be a bit harder. But I think we can figure it out with "detached" mode. Or maybe we'd just always require related components to call WithLightbox internally for now.

The important question here: do we currently clone DOM elements to put them inside the lightbox? Or do we reparent them? Or just position? This is important, because if we clone/reposition, then in React that'd probably be taken care of by portalling, which we haven't worked with yet here, but could open some doors for us.

[caroqliu]

We can get nested children if we are willing to clone elements, but this is costly and somewhat hacky to drill it down like this:

<LightboxGalleryContext.Provider value={context}>
  <WithDeepLightbox>{children}</WithDeepLightbox>
</LightboxGalleryContext.Provider>

function WithDeepLightbox({children}) {
  return toChildArray(children).map((child) =>
    child.props?.lightbox ? (
      <WithLightbox>{child}</WithLightbox>
    ) : child.props?.children ? (
      Preact.cloneElement(child, null, <WithDeepLightbox>{child.props.children}</WithDeepLightbox>)
    ) : (
      child
    )
  );
}

To your point about portals - 0.1 clones and cleans lightboxable DOM elements:

amphtml/extensions/amp-lightbox-gallery/0.1/amp-lightbox-gallery.js

Lines 269 to 290 in ea2e7f6

	/**
	* Return a cleaned clone of the given element for building
	* carousel slides with.
	* @param {!Element} element
	* @return {!Node}
	* @private
	*/
	cloneLightboxableElement_(element) {
	const fallback = element.getFallback();
	const shouldCloneFallback =
	element.classList.contains('amp-notsupported') && !!fallback;
	if (shouldCloneFallback) {
	element = fallback;
	}
	const deepClone = !element.classList.contains('i-amphtml-element');
	const clonedNode = element.cloneNode(deepClone);
	clonedNode.removeAttribute('on');
	clonedNode.removeAttribute('id');
	clonedNode.removeAttribute('i-amphtml-layout');
	clonedNode.removeAttribute('fallback');
	return clonedNode;
	}

Can you describe more how portals helps this use case? I imagine we would want lightboxed elements both to be rendered where they are in the DOM as well as in the LightboxGallery when it is opened. From what I've read about portals, it doesn't seem like you can have it both ways?

Edit: Discussed portals offline and prefer taking render prop for inside the lightbox.

[lgtm-com]

This pull request introduces 1 alert when merging 752fea6 into afde3c0 - view on LGTM.com

new alerts:

• 1 for Useless assignment to property

[dvoytenko]

Do we need to graft it on top of content? Or maybe it's better to just put WithLightbox as a child? In the later comments I was also wondering if we could just wrap the WithLightbox around each slide.

[caroqliu]

Don't necessarily need this, mostly wanted to reduce a layer of parenting in the tree. We can put WithLightbox as a child for more readability, but I wonder - would the carousel have different # of intermediary layers if lightbox is true (+1) versus false? And would that be weird?

[dvoytenko]

I'm curious if it'd make sense to make WithLightbox to wrap each slide inside for lightboxable cases? The WithLightbox, it looks like, already implements something like this.

[caroqliu]

Discussed offline, there are a few reasons this approach is preferred to wrapping each slide:

• Partial rendering inside BaseCarousel: For the portion of slides that are not rendered, they also would not be registered with the LightboxGallery, so if it were to open, you would only see images for some of the lightbox consumers, which is not desirable for the grid view.
• Slide reordering inside BaseCarousel: Looping slides are rendered in a different order than given. This means that if you give slides A, B, C and carousel renders them as B, C, A for looping, the lightbox gallery would then also receive them in B, C, A, order, even though - again for grid view - they should be registered to the LightboxGallery in the same preserved order in which they are given to the BaseCarousel.

For these reasons, wrapping each slide leaks implementation details.

However, this opens up the next point of investigation: Is it okay to reuse JSX? Because BaseCarousel provides children to WithLightbox both as children and render={() => children}, we need to understand the full consequences of using them this way. i.e. What happens to refs?

[dvoytenko]

To clarify: in this model we never need a placeholder, right?

[caroqliu]

Not sure what you are referring to?

[dvoytenko]

In other words: do we ever have a UX where the original image in the document is hidden and only showing blank "gray" space?

[caroqliu]

Yes, this is possible because the LightboxGallery does not control the content of the consumer. But I am not sure why this matters?