StreamGallery: Initial implementation #30597
Conversation
| if (!galleryRef.current) { | ||
| return {visibleCount: 1, maxContainerWidth: Number.MAX_VALUE}; | ||
| } | ||
| const width = galleryRef.current./* OK */ offsetWidth; |
There was a problem hiding this comment.
Hmm. This is a bit of a trouble. It's not a code we can run on SSR side. And also means that we need to listen to resizes. I understand that in SSR mode it should never get this far b/c of the if above. But it feels cryptic. Also, might be a source of CLS violations. Any other way to express it? At the very least we could set it up as a state+effect-with-resize like fit-text.
There was a problem hiding this comment.
I refactored this a bit, could you take another look and clarify concerns if they remain?
| // ignore ResizeObserver loop limit exceeded | ||
| // this is ok in several scenarios according to | ||
| // https://github.com/WICG/resize-observer/issues/38 | ||
| before(() => { | ||
| window.onerror = function (err) { | ||
| if (err === 'ResizeObserver loop limit exceeded') { | ||
| return false; | ||
| } else { | ||
| return err; | ||
| } | ||
| }; | ||
| }); | ||
|
|
There was a problem hiding this comment.
This is where the error is ignored @dvoytenko
| ); | ||
|
|
||
| // Adjust visible slide count when container size or parameters change. | ||
| useLayoutEffect(() => { |
There was a problem hiding this comment.
I believe there is a subtle hidden bug in this layoutEffect hook.
Namely that changes to the galleryRef.current pointer would not retrigger the effect.
If it is working, it is because we are getting lucky with the synchronization of the ref setting and when the render is happening.
The solution is to use a callback ref.
In this case, I think that means:
const setupMeasureRef = useCallback(() => { ... }, [measure]);
return <div ref={setupMeasureRef}> ... </div>;There was a problem hiding this comment.
I just removed the outer div with the ref in favor of using one provided by the BaseCarousel, does this comment still apply?
There was a problem hiding this comment.
The issue that useLayoutEffect will not be notified about a change to ref.current.node still exists. The way the code is currently written, I don't think it would be a problem since the nodes on which the refs exist are static (BaseCarousel / ScrollerRef).
If in the future the ref moves to a component or node that is in any way rendered conditionally, then it becomes a bug. If this is a safe assumption, feel free to leave as-is
There was a problem hiding this comment.
The DOM ref changes is a big headache in React. They are not directly supported by any API. I think the overall idea is, as much as possible, to assume DOM refs do not change unless it's an explicitly supported case.
| ); | ||
|
|
||
| // Adjust visible slide count when container size or parameters change. | ||
| useLayoutEffect(() => { |
There was a problem hiding this comment.
The DOM ref changes is a big headache in React. They are not directly supported by any API. I think the overall idea is, as much as possible, to assume DOM refs do not change unless it's an explicitly supported case.
| measurements.maxContainerWidth >= Number.MAX_VALUE | ||
| ? '' | ||
| : measurements.maxContainerWidth, | ||
| justifyContent: extraSpace === 'around' ? 'center' : 'initial', |
There was a problem hiding this comment.
This might be an important issue. It's hard to use flexbox in styles because the vendor-prefixing is very complicated. Is there a way to push flexGrow, justifyContent, and similar to JSS? We can always allow a mode on BaseCarousel and let it apply the stylesheet class.
There was a problem hiding this comment.
Moved these to JSS, PTAL.
* Add Storybook samples * Prototype Preact StreamGallery * Prototype AMP component * Allow stream gallery to import from base carousel * Add unit tests * Remove slideAlign for now * Sync with latest BaseCarousel changes * Use ResizeObserver * Update comment note * Shrink arrow JSX * Do not export `Controls` * Update tests * Add guard in Scroller class * Update test for third slide being rendered * Disallow infinity maxWidth value * Guard for ResizeObserver testing error * Clean up logic * Add AMP CSS and test * Rename `width` to `containerWidth` * Use ResizeObserver entry to measure container * Reset window.onerror * Remove after hook for now * Update dependency checks * Use ref from scroller * Prefer ?? to || * Reset window.onerror * Return the ref.current.node as the root element * Inline JSX * Use the ResizeObserver from the local window * Guard for ref.current * Move styles to classes * getVisibleCount should return visibleCount * Scroller node not needed * Fix types * Update dependency allowlist * Resolve extraSpace class * Destructure props in function body
This PR adds a
StreamGalleryPreact component and registers itsamp-stream-gallerycounterpart. Partial for #28284.Key takeaways:
StreamGallerytakes a number of props and, along with the size of its container, calculates avisibleCountof slides to show at once.BaseCarouselunder the hood. This decreases the implementation surface area significantly.StreamGallery:minItemWidth,maxItemWidth,minVisibleCount,maxVisibleCount, andpeek-- these are taken together, along with the number of items and dimensions of the component, to calculate thevisibleCountdescribed above. This is then passed to the underlyingBaseCarousel.BaseCarouselfor this component to work:visibleCountsnapAlign,advanceCountIn 0.1,EDIT:amp-stream-gallerytook an attribute calledoutset-arrowsto allow displaying the carousel arrows on either side of the carousel rather than overlaid on top of the slides. I'd like to propose that this attribute also be added toamp-base-carouselso that this can be configurable at that layer rather than unique toamp-stream-gallery. -- This PR demonstrates the additional logic required (given toamp-base-carouselif inset, and provided separately ifoutset) if we do not do this.outset-arrowshas been added toamp-base-carouselso foramp-stream-gallery, it has become yet another prop to pass through.In a follow up PR I will be adding
snapAligntoBaseCarouselandslideAligntoStreamGallery.