From 39fd3238ef6c89ff669f089ebf05572c5af614b5 Mon Sep 17 00:00:00 2001 From: Caroline Liu <10456171+caroqliu@users.noreply.github.com> Date: Wed, 2 Dec 2020 16:59:19 -0500 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20Bento=20Carousel:=20"orientation"?= =?UTF-8?q?=20feature=20(#31286)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add Storybook samples * Support "orientation" attr/prop * Add e2e test * Code cleanups * Add inline gallery Storybook usage * 2019 -> 2020 * Update styling for mixedLength + vertical * Remove slide `height: 100%` entirely * Remove axis state * Round all dimensions --- .../1.0/amp-base-carousel.js | 6 + .../amp-base-carousel/1.0/base-carousel.js | 6 +- .../1.0/base-carousel.jss.js | 11 +- .../1.0/base-carousel.type.js | 2 + .../amp-base-carousel/1.0/dimensions.js | 16 ++- extensions/amp-base-carousel/1.0/scroller.js | 13 +- .../1.0/storybook/Basic.amp.js | 26 ++-- .../amp-base-carousel/1.0/storybook/Basic.js | 24 +++- .../1.0/test-e2e/test-vertical.js | 130 ++++++++++++++++++ .../1.0/storybook/Basic.amp.js | 8 +- .../amp-inline-gallery/1.0/storybook/Basic.js | 8 +- .../amp-base-carousel/1.0/vertical.amp.html | 33 +++++ 12 files changed, 253 insertions(+), 30 deletions(-) create mode 100644 extensions/amp-base-carousel/1.0/test-e2e/test-vertical.js create mode 100644 test/manual/amp-base-carousel/1.0/vertical.amp.html diff --git a/extensions/amp-base-carousel/1.0/amp-base-carousel.js b/extensions/amp-base-carousel/1.0/amp-base-carousel.js index 342180faceb1..12e6b0226579 100644 --- a/extensions/amp-base-carousel/1.0/amp-base-carousel.js +++ b/extensions/amp-base-carousel/1.0/amp-base-carousel.js @@ -100,6 +100,12 @@ AmpBaseCarousel['props'] = { }, 'autoAdvanceLoops': {attr: 'auto-advance-loops', type: 'number', media: true}, 'controls': {attr: 'controls', type: 'string', media: true}, + 'orientation': { + attr: 'orientation', + type: 'string', + media: true, + default: 'horizontal', + }, 'loop': {attr: 'loop', type: 'boolean', media: true}, 'mixedLength': {attr: 'mixed-length', type: 'boolean', media: true}, 'outsetArrows': {attr: 'outset-arrows', type: 'boolean', media: true}, diff --git a/extensions/amp-base-carousel/1.0/base-carousel.js b/extensions/amp-base-carousel/1.0/base-carousel.js index ba0d8669d38c..a1037f6df891 100644 --- a/extensions/amp-base-carousel/1.0/base-carousel.js +++ b/extensions/amp-base-carousel/1.0/base-carousel.js @@ -14,7 +14,7 @@ * limitations under the License. */ import * as Preact from '../../../src/preact'; -import {Alignment} from './dimensions'; +import {Alignment, Axis, Orientation} from './dimensions'; import {Arrow} from './arrow'; import {CarouselContext} from './carousel-context'; import {ContainWrapper} from '../../../src/preact/component'; @@ -88,6 +88,7 @@ function BaseCarouselWithRef( onMouseEnter, onSlideChange, onTouchStart, + orientation = Orientation.HORIZONTAL, outsetArrows, snap = true, snapAlign = Alignment.START, @@ -107,9 +108,11 @@ function BaseCarouselWithRef( const setCurrentSlide = carouselContext.setCurrentSlide ?? setCurrentSlideState; const {slides, setSlides} = carouselContext; + const scrollRef = useRef(null); const containRef = useRef(null); const contentRef = useRef(null); + const autoAdvanceTimesRef = useRef(0); const autoAdvanceInterval = useMemo( () => Math.max(customAutoAdvanceInterval, MIN_AUTO_ADVANCE_INTERVAL), @@ -261,6 +264,7 @@ function BaseCarouselWithRef( advanceCount={advanceCount} alignment={snapAlign} autoAdvanceCount={autoAdvanceCount} + axis={orientation == Orientation.HORIZONTAL ? Axis.X : Axis.Y} loop={loop} mixedLength={mixedLength} restingIndex={currentSlide} diff --git a/extensions/amp-base-carousel/1.0/base-carousel.jss.js b/extensions/amp-base-carousel/1.0/base-carousel.jss.js index 98dcb77179ca..474f39c72b09 100644 --- a/extensions/amp-base-carousel/1.0/base-carousel.jss.js +++ b/extensions/amp-base-carousel/1.0/base-carousel.jss.js @@ -43,6 +43,13 @@ const horizontalScroll = { }, }; +const verticalScroll = { + flexDirection: 'column', + scrollSnapTypeY: 'mandatory', // Firefox/IE + scrollSnapType: 'y mandatory', + overflowX: 'hidden', +}; + /* * Styles to hide scrollbars, with three different methods: * @@ -61,8 +68,6 @@ const hideScrollbar = { scrollbarWidth: 'none', boxSizing: '', - height: '100%', - paddingBottom: '20px', // Chrome, Safari '&::-webkit-scrollbar': { @@ -72,7 +77,6 @@ const hideScrollbar = { }; const slideElement = { - height: '100%', position: 'relative', overflow: 'hidden', display: 'flex', @@ -247,6 +251,7 @@ const JSS = { scrollContainer, hideScrollbar, horizontalScroll, + verticalScroll, slideElement, thumbnails, enableSnap, diff --git a/extensions/amp-base-carousel/1.0/base-carousel.type.js b/extensions/amp-base-carousel/1.0/base-carousel.type.js index 85b781c5790a..6ca0177ebc41 100644 --- a/extensions/amp-base-carousel/1.0/base-carousel.type.js +++ b/extensions/amp-base-carousel/1.0/base-carousel.type.js @@ -33,6 +33,7 @@ var BaseCarouselDef = {}; * loop: (boolean|undefined), * mixedLength: (boolean|undefined), * onSlideChange: (function(number):undefined|undefined), + * orientation: (string|undefined), * snap: (boolean|undefined), * snapAlign: (string|undefined), * snapBy: (number|undefined), @@ -44,6 +45,7 @@ BaseCarouselDef.Props; /** * @typedef {{ * advanceCount: (number|undefined), + * axis: number, * children: !Array, * loop: (boolean|undefined), * mixedLength: (boolean|undefined), diff --git a/extensions/amp-base-carousel/1.0/dimensions.js b/extensions/amp-base-carousel/1.0/dimensions.js index db6924116da2..62a959b0526d 100644 --- a/extensions/amp-base-carousel/1.0/dimensions.js +++ b/extensions/amp-base-carousel/1.0/dimensions.js @@ -32,6 +32,14 @@ export const Alignment = { CENTER: 'center', }; +/** + * @enum {string} + */ +export const Orientation = { + HORIZONTAL: 'horizontal', + VERTICAL: 'vertical', +}; + /** * @typedef {{ * start: number, @@ -57,9 +65,9 @@ export function getDimension(axis, el) { } = el./*OK*/ getBoundingClientRect(); return { - start: axis == Axis.X ? left : top, - end: axis == Axis.X ? right : bottom, - length: axis == Axis.X ? width : height, + start: Math.round(axis == Axis.X ? left : top), + end: Math.round(axis == Axis.X ? right : bottom), + length: Math.round(axis == Axis.X ? width : height), }; } @@ -105,7 +113,7 @@ export function getPosition(axis, alignment, el) { export function overlaps(axis, el, position) { const {start, end} = getDimension(axis, el); // Ignore the end point, since that is shared with the adjacent Element. - return Math.round(start) <= position && position < Math.round(end); + return start <= position && position < end; } /** diff --git a/extensions/amp-base-carousel/1.0/scroller.js b/extensions/amp-base-carousel/1.0/scroller.js index dac6f07485f5..dc7de9aa43f2 100644 --- a/extensions/amp-base-carousel/1.0/scroller.js +++ b/extensions/amp-base-carousel/1.0/scroller.js @@ -31,7 +31,6 @@ import { useLayoutEffect, useMemo, useRef, - useState, } from '../../../src/preact'; import {useStyles} from './base-carousel.jss'; @@ -56,6 +55,7 @@ function ScrollerWithRef( advanceCount, alignment, autoAdvanceCount, + axis, children, loop, mixedLength, @@ -71,7 +71,6 @@ function ScrollerWithRef( ) { // We still need our own ref that we can always rely on to be there. const containerRef = useRef(null); - const [axis] = useState(Axis.X); /** * The number of slides we want to place before the reference or resting index. @@ -240,7 +239,9 @@ function ScrollerWithRef(
@@ -332,8 +333,10 @@ function renderSlides( snap && mod(index, snapBy) === 0 ? classes.enableSnap : classes.disableSnap - } ${_thumbnails ? classes.thumbnails : ''}`} - style={{flex: mixedLength ? '0 0 auto' : `0 0 ${100 / visibleCount}%`}} + } ${_thumbnails ? classes.thumbnails : ''} `} + style={{ + flex: mixedLength ? '0 0 auto' : `0 0 ${100 / visibleCount}%`, + }} > {child}
diff --git a/extensions/amp-base-carousel/1.0/storybook/Basic.amp.js b/extensions/amp-base-carousel/1.0/storybook/Basic.amp.js index bdc5af2160e9..954c406d09a6 100644 --- a/extensions/amp-base-carousel/1.0/storybook/Basic.amp.js +++ b/extensions/amp-base-carousel/1.0/storybook/Basic.amp.js @@ -19,6 +19,8 @@ import {boolean, number, select, text, withKnobs} from '@storybook/addon-knobs'; import {withA11y} from '@storybook/addon-a11y'; import {withAmp} from '@ampproject/storybook-addon'; +const ORIENTATIONS = ['horizontal', 'vertical']; + export default { title: 'amp-base-carousel', decorators: [withKnobs, withA11y, withAmp], @@ -42,6 +44,7 @@ export const Default = () => { const visibleCount = text('visible count', '(min-width: 400px) 2, 1'); const outsetArrows = text('outset arrows', '(min-width: 400px) true, false'); const controls = select('show controls', ['auto', 'always', 'never']); + const orientation = select('orientation', ORIENTATIONS, 'vertical'); const slideCount = number('slide count', 5, {min: 0, max: 99}); const colorIncrement = Math.floor(255 / (slideCount + 1)); @@ -55,20 +58,20 @@ export const Default = () => { auto-advance-interval={autoAdvanceInterval} auto-advance-loops={autoAdvanceLoops} controls={controls} + orientation={orientation} outset-arrows={outsetArrows} - width="880" - height="225" + width="450" + height="450" snap={String(snap)} snap-align={snapAlign} snap-by={snapBy} loop={loop} - layout="responsive" visible-count={visibleCount} > {Array.from({length: slideCount}, (x, i) => { const v = colorIncrement * (i + 1); return ( - +
{ }; export const mixedLength = () => { - const width = number('width', 440); - const height = number('height', 225); + const width = number('width', 300); + const height = number('height', 300); const slideCount = number('slide count', 7, {min: 0, max: 99}); const colorIncrement = Math.floor(255 / (slideCount + 1)); const loop = boolean('loop', true); @@ -113,12 +116,15 @@ export const mixedLength = () => { [252, 113, 115, 186, 248, 188, 162, 104, 100, 109, 175, 227, 143, 249, 280], ]; const preset = select('random preset', [1, 2, 3]); + const orientation = select('orientation', ORIENTATIONS, 'vertical'); + const horizontal = orientation == 'horizontal'; return ( { style={{ backgroundColor: `rgb(${v}, 100, 100)`, border: 'solid white 1px', - width: `${randomPreset[preset - 1 || 0][i]}px`, - height: `100px`, + width: horizontal + ? `${randomPreset[preset - 1 || 0][i]}px` + : '100px', + height: horizontal + ? '100px' + : `${randomPreset[preset - 1 || 0][i]}px`, }} >
); diff --git a/extensions/amp-base-carousel/1.0/storybook/Basic.js b/extensions/amp-base-carousel/1.0/storybook/Basic.js index 73fe16d6567b..eb7f1dc96f38 100644 --- a/extensions/amp-base-carousel/1.0/storybook/Basic.js +++ b/extensions/amp-base-carousel/1.0/storybook/Basic.js @@ -21,6 +21,7 @@ import {withA11y} from '@storybook/addon-a11y'; const CONTROLS = ['auto', 'always', 'never']; const SNAP_ALIGN = ['start', 'center']; +const ORIENTATIONS = ['horizontal', 'vertical']; export default { title: 'BaseCarousel', @@ -49,12 +50,13 @@ function CarouselWithActions(props) { } export const _default = () => { - const width = number('width', 440); - const height = number('height', 225); + const width = number('width', 225); + const height = number('height', 440); const slideCount = number('slide count', 5, {min: 0, max: 99}); const snap = boolean('snap', true); const snapAlign = select('snap alignment', SNAP_ALIGN, 'start'); const snapBy = number('snap by', 1); + const orientation = select('orientation', ORIENTATIONS, 'vertical'); const loop = boolean('loop', true); const advanceCount = number('advance count', 1, {min: 1}); const visibleCount = number('visible count', 2, {min: 1}); @@ -66,6 +68,7 @@ export const _default = () => { advanceCount={advanceCount} controls={controls} loop={loop} + orientation={orientation} outsetArrows={outsetArrows} snap={snap} snapAlign={snapAlign} @@ -83,7 +86,7 @@ export const _default = () => { height, textAlign: 'center', fontSize: '48pt', - lineHeight: height + 'px', + lineHeight: height / visibleCount + 'px', }} > {i} @@ -95,8 +98,8 @@ export const _default = () => { }; export const mixedLength = () => { - const width = number('width', 440); - const height = number('height', 225); + const width = number('width', 300); + const height = number('height', 300); const slideCount = 15; const colorIncrement = Math.floor(255 / (slideCount + 1)); const autoAdvance = boolean('auto advance', true); @@ -115,6 +118,8 @@ export const mixedLength = () => { [252, 113, 115, 186, 248, 188, 162, 104, 100, 109, 175, 227, 143, 249, 280], ]; const preset = select('random preset', [1, 2, 3]); + const orientation = select('orientation', ORIENTATIONS, 'vertical'); + const horizontal = orientation == 'horizontal'; return ( { controls={controls} mixedLength={mixedLength} loop={loop} + orientation={orientation} snap={snap} snapAlign={snapAlign} snapBy={snapBy} @@ -136,8 +142,12 @@ export const mixedLength = () => { style={{ backgroundColor: `rgb(${v}, 100, 100)`, border: 'solid white 1px', - width: `${randomPreset[preset - 1 || 0][i]}px`, - height: `100px`, + width: horizontal + ? `${randomPreset[preset - 1 || 0][i]}px` + : '100px', + height: horizontal + ? '100px' + : `${randomPreset[preset - 1 || 0][i]}px`, }} > ); diff --git a/extensions/amp-base-carousel/1.0/test-e2e/test-vertical.js b/extensions/amp-base-carousel/1.0/test-e2e/test-vertical.js new file mode 100644 index 000000000000..3f004780dfb8 --- /dev/null +++ b/extensions/amp-base-carousel/1.0/test-e2e/test-vertical.js @@ -0,0 +1,130 @@ +/** + * Copyright 2020 The AMP HTML Authors. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS-IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import {getCarousel, getScrollingElement, getSlide} from './helpers'; +import {useStyles} from '../base-carousel.jss'; + +const pageWidth = 600; +const pageHeight = 600; + +describes.endtoend( + 'Vertical AMP carousel', + { + testUrl: + 'http://localhost:8000/test/manual/amp-base-carousel/1.0/' + + 'vertical.amp.html', + experiments: ['amp-base-carousel-bento'], + initialRect: {width: pageWidth, height: pageHeight}, + environments: ['single', 'viewer-demo'], + }, + async (env) => { + /** The total number of slides in the carousel */ + const SLIDE_COUNT = 7; + let controller; + + const styles = useStyles(); + + function prop(el, name) { + return controller.getElementProperty(el, name); + } + + beforeEach(async () => { + controller = env.controller; + const carousel = await getCarousel(controller); + await controller.switchToShadowRoot(carousel); + }); + + it('should render correctly', async () => { + const el = await getScrollingElement(styles, controller); + const firstSlide = await getSlide(styles, controller, 0); + + // We should have space for all slides + const slideHeight = await prop(firstSlide, 'offsetHeight'); + await expect(prop(el, 'scrollHeight')).to.equal( + slideHeight * SLIDE_COUNT + ); + }); + + it('should snap when scrolling', async () => { + const el = await getScrollingElement(styles, controller); + const firstSlide = await getSlide(styles, controller, 0); + + const slideHeight = await prop(firstSlide, 'offsetHeight'); + const scrollTop = await prop(el, 'scrollTop'); + const snappedScrollTop = scrollTop + slideHeight; + const requestedScrollTop = snappedScrollTop + 1; + + await controller.scrollTo(el, {top: requestedScrollTop}); + // We should have snapped to the edge of the slide rather than the + // requested scroll position. + await expect(prop(el, 'scrollTop')).to.equal(snappedScrollTop); + }); + + it('should reset the window after scroll', async function () { + const el = await getScrollingElement(styles, controller); + const firstSlide = await getSlide(styles, controller, 0); + + const slideHeight = await prop(firstSlide, 'offsetHeight'); + const scrollHeight = await prop(el, 'scrollHeight'); + const scrollTop = await prop(el, 'scrollTop'); + const snappedScrollTop = scrollTop + slideHeight; + const requestedScrollTop = snappedScrollTop + 1; + + await controller.scrollTo(el, {top: requestedScrollTop}); + // Wait for the scrolling to settle + await expect(prop(el, 'scrollTop')).to.equal(snappedScrollTop); + + // The new scroll width/left should eventually be the same as before, + // since the windowing should have been reset around the new element. + await expect(prop(el, 'scrollHeight')).to.equal(scrollHeight); + await expect(prop(el, 'scrollTop')).to.equal(scrollTop); + }); + + describe('looping', () => { + it('should display slides correctly when moving forwards', async () => { + const el = await getScrollingElement(styles, controller); + const lastSlide = await getSlide(styles, controller, SLIDE_COUNT - 1); + + // Go to the last slide, wait for scrolling to move. + const slideHeight = await prop(lastSlide, 'offsetHeight'); + const restingScrollTop = await prop(el, 'scrollTop'); + await controller.scrollBy(el, { + top: slideHeight * (SLIDE_COUNT - 1), + }); + + await expect(prop(el, 'scrollTop')).to.equal(restingScrollTop); + await expect(controller.getElementRect(lastSlide)).to.include({ + height: slideHeight, + }); + }); + + it('should display slides correctly when moving backwards', async () => { + const el = await getScrollingElement(styles, controller); + const secondSlide = await getSlide(styles, controller, 1); + + // Go to the last slide, wait for scrolling to move. + const slideHeight = await prop(secondSlide, 'offsetHeight'); + const restingScrollTop = await prop(el, 'scrollTop'); + await controller.scrollBy(el, { + top: -(slideHeight * (SLIDE_COUNT - 1)), + }); + + await expect(prop(el, 'scrollTop')).to.not.equal(restingScrollTop); + await expect(prop(el, 'scrollTop')).to.equal(restingScrollTop); + }); + }); + } +); diff --git a/extensions/amp-inline-gallery/1.0/storybook/Basic.amp.js b/extensions/amp-inline-gallery/1.0/storybook/Basic.amp.js index 40813e10ad24..f0159d8cec29 100644 --- a/extensions/amp-inline-gallery/1.0/storybook/Basic.amp.js +++ b/extensions/amp-inline-gallery/1.0/storybook/Basic.amp.js @@ -15,7 +15,7 @@ */ import * as Preact from '../../../../src/preact'; -import {boolean, text, withKnobs} from '@storybook/addon-knobs'; +import {boolean, select, text, withKnobs} from '@storybook/addon-knobs'; import {withA11y} from '@storybook/addon-a11y'; import {withAmp} from '@ampproject/storybook-addon'; @@ -42,6 +42,11 @@ export const Default = () => { const autoAdvanceLoops = text('auto advance loops', 3); const loop = boolean('loop thumbnails', false); const aspectRatio = text('thumbnails aspect ratio', undefined); + const orientation = select( + 'orientation', + ['horizontal', 'vertical'], + 'vertical' + ); return ( { auto-advance-count={autoAdvanceCount} auto-advance-interval={autoAdvanceInterval} auto-advance-loops={autoAdvanceLoops} + orientation={orientation} width="360" height="240" > diff --git a/extensions/amp-inline-gallery/1.0/storybook/Basic.js b/extensions/amp-inline-gallery/1.0/storybook/Basic.js index d31a4408c761..1a38ac9aea81 100644 --- a/extensions/amp-inline-gallery/1.0/storybook/Basic.js +++ b/extensions/amp-inline-gallery/1.0/storybook/Basic.js @@ -19,7 +19,7 @@ import {BaseCarousel} from '../../../amp-base-carousel/1.0/base-carousel'; import {InlineGallery} from '../inline-gallery'; import {Pagination} from '../pagination'; import {Thumbnails} from '../thumbnails'; -import {boolean, number, withKnobs} from '@storybook/addon-knobs'; +import {boolean, number, select, withKnobs} from '@storybook/addon-knobs'; import {withA11y} from '@storybook/addon-a11y'; export default { @@ -41,6 +41,11 @@ export const _default = () => { const thumbnailHeight = number('thumbnail height', 50); const loop = boolean('thumbnail loop', false); const aspectRatio = number('thumbnail aspect ratio (w/h)', 3 / 2); + const orientation = select( + 'orientation', + ['horizontal', 'vertical'], + 'vertical' + ); return ( <> @@ -52,6 +57,7 @@ export const _default = () => { autoAdvanceInterval={autoAdvanceInterval} autoAdvanceLoops={autoAdvanceLoops} autoAdvance={autoAdvance} + orientation={orientation} > + + + + Vertical Carousel + + + + + + + + + +

A vertical carousel

+ + + + + + + + + + + + + +