Bento Carousel: Support passing in custom arrows with as (#33636)

* Update Storybooks * Pass in arrowPrevAs and arrowNextAs * Give arrowPrevAs and arrowNextAs from AMP layer * Update unit test * Update for stream gallery * Use objstr * Clone element preferred * amp-stream-gallery should also cloneElement
ampproject · Apr 19, 2021 · fc7b0ed · fc7b0ed
1 parent 5b63d8b
commit fc7b0ed
Show file tree

Hide file tree

Showing 10 changed files with 203 additions and 102 deletions.
diff --git a/extensions/amp-base-carousel/1.0/amp-base-carousel.js b/extensions/amp-base-carousel/1.0/amp-base-carousel.js
@@ -21,6 +21,7 @@ import {CSS} from '../../../build/amp-base-carousel-1.0.css';
 import {CarouselContextProp} from './carousel-props';
 import {PreactBaseElement} from '../../../src/preact/base-element';
 import {Services} from '../../../src/services';
+import {cloneElement} from '../../../src/preact';
 import {createCustomEvent} from '../../../src/event-helper';
 import {dict} from '../../../src/core/types/object';
 import {dispatchCustomEvent} from '../../../src/dom';
@@ -84,6 +85,17 @@ class AmpBaseCarousel extends PreactBaseElement {
       this.api().goToSlide(slide);
     }
   }
+
+  /** @override */
+  updatePropsForRendering(props) {
+    const {arrowPrev, arrowNext} = props;
+    if (arrowPrev) {
+      props['arrowPrevAs'] = (props) => cloneElement(arrowPrev, props);
+    }
+    if (arrowNext) {
+      props['arrowNextAs'] = (props) => cloneElement(arrowNext, props);
+    }
+  }
 }
 
 /** @override */

diff --git a/extensions/amp-base-carousel/1.0/arrow.js b/extensions/amp-base-carousel/1.0/arrow.js
@@ -16,77 +16,86 @@
 
 import * as Preact from '../../../src/preact';
 import {useStyles} from './base-carousel.jss';
+import objstr from 'obj-str';
 
 /**
  * @param {!BaseCarouselDef.ArrowProps} props
  * @return {PreactDef.Renderable}
  */
 export function Arrow({
   advance,
+  as: Comp = DefaultArrow,
   by,
-  customArrow = <DefaultArrow by={by} />,
   disabled,
   outsetArrows,
   rtl,
+  ...rest
 }) {
-  const {
-    'disabled': customDisabled,
-    'onClick': onCustomClick,
-  } = customArrow.props;
-  const isDisabled = disabled || customDisabled;
-  const onClick = (e) => {
-    if (isDisabled) {
-      return;
-    }
-    if (onCustomClick) {
-      onCustomClick(e);
+  const classes = useStyles();
+  const onClick = () => {
+    if (!disabled) {
+      advance();
     }
-    advance();
   };
-  const classes = useStyles();
-  const classNames = `${classes.arrow} ${
-    by < 0 ? classes.arrowPrev : classes.arrowNext
-  } ${isDisabled ? classes.arrowDisabled : ''} ${
-    outsetArrows ? classes.outsetArrow : classes.insetArrow
-  } ${rtl ? classes.rtl : classes.ltr}`;
-
   return (
-    <div class={classNames}>
-      {Preact.cloneElement(customArrow, {
-        'onClick': onClick,
-        'disabled': isDisabled,
-        'aria-disabled': String(!!isDisabled),
+    <Comp
+      aria-disabled={String(!!disabled)}
+      by={by}
+      className={objstr({
+        [classes.arrow]: true,
+        [classes.arrowDisabled]: disabled,
+        [classes.arrowPrev]: by < 0,
+        [classes.arrowNext]: by > 0,
+        [classes.outsetArrow]: outsetArrows,
+        [classes.insetArrow]: !outsetArrows,
+        [classes.rtl]: rtl,
+        [classes.ltr]: !rtl,
       })}
-    </div>
+      disabled={disabled}
+      onClick={onClick}
+      outsetArrows={outsetArrows}
+      rtl={rtl}
+      {...rest}
+    />
   );
 }
 
 /**
  * @param {!BaseCarouselDef.ArrowProps} props
  * @return {PreactDef.Renderable}
  */
-function DefaultArrow({by, ...rest}) {
+function DefaultArrow({by, className, ...rest}) {
   const classes = useStyles();
   return (
-    <button
-      class={classes.defaultArrowButton}
-      aria-label={
-        by < 0 ? 'Previous item in carousel' : 'Next item in carousel'
-      }
-      {...rest}
-    >
-      <div class={`${classes.arrowBaseStyle} ${classes.arrowFrosting}`}></div>
-      <div class={`${classes.arrowBaseStyle} ${classes.arrowBackdrop}`}></div>
-      <div class={`${classes.arrowBaseStyle} ${classes.arrowBackground}`}></div>
-      <svg class={classes.arrowIcon} viewBox="0 0 24 24">
-        <path
-          d={by < 0 ? 'M14,7.4 L9.4,12 L14,16.6' : 'M10,7.4 L14.6,12 L10,16.6'}
-          fill="none"
-          stroke-width="2px"
-          stroke-linejoin="round"
-          stroke-linecap="round"
-        />
-      </svg>
-    </button>
+    <div className={className}>
+      <button
+        aria-label={
+          by < 0 ? 'Previous item in carousel' : 'Next item in carousel'
+        }
+        className={classes.defaultArrowButton}
+        {...rest}
+      >
+        <div
+          className={`${classes.arrowBaseStyle} ${classes.arrowFrosting}`}
+        ></div>
+        <div
+          className={`${classes.arrowBaseStyle} ${classes.arrowBackdrop}`}
+        ></div>
+        <div
+          className={`${classes.arrowBaseStyle} ${classes.arrowBackground}`}
+        ></div>
+        <svg className={classes.arrowIcon} viewBox="0 0 24 24">
+          <path
+            d={
+              by < 0 ? 'M14,7.4 L9.4,12 L14,16.6' : 'M10,7.4 L14.6,12 L10,16.6'
+            }
+            fill="none"
+            stroke-width="2px"
+            stroke-linejoin="round"
+            stroke-linecap="round"
+          />
+        </svg>
+      </button>
+    </div>
   );
 }
diff --git a/extensions/amp-base-carousel/1.0/base-carousel.js b/extensions/amp-base-carousel/1.0/base-carousel.js
@@ -83,8 +83,8 @@ const MIN_AUTO_ADVANCE_INTERVAL = 1000;
 function BaseCarouselWithRef(
   {
     advanceCount = 1,
-    arrowPrev,
-    arrowNext,
+    arrowPrevAs,
+    arrowNextAs,
     autoAdvance: shouldAutoAdvance = false,
     autoAdvanceCount = 1,
     autoAdvanceInterval: customAutoAdvanceInterval = MIN_AUTO_ADVANCE_INTERVAL,
@@ -101,7 +101,7 @@ function BaseCarouselWithRef(
     onSlideChange,
     onTouchStart,
     orientation = Orientation.HORIZONTAL,
-    outsetArrows,
+    outsetArrows = false,
     snap = true,
     snapAlign = Alignment.START,
     snapBy = 1,
@@ -330,8 +330,8 @@ function BaseCarouselWithRef(
       {!hideControls && (
         <Arrow
           advance={prev}
+          as={arrowPrevAs}
           by={-advanceCount}
-          customArrow={arrowPrev}
           disabled={disableForDir(-1)}
           outsetArrows={outsetArrows}
           rtl={rtl}
@@ -387,7 +387,7 @@ function BaseCarouselWithRef(
         <Arrow
           advance={next}
           by={advanceCount}
-          customArrow={arrowNext}
+          as={arrowNextAs}
           disabled={disableForDir(1)}
           outsetArrows={outsetArrows}
           rtl={rtl}

diff --git a/extensions/amp-base-carousel/1.0/storybook/Basic.amp.js b/extensions/amp-base-carousel/1.0/storybook/Basic.amp.js
@@ -161,4 +161,30 @@ export const mixedLength = () => {
   );
 };
 
+export const customArrows = () => {
+  const width = number('width', 400);
+  const height = number('height', 200);
+  const slideCount = number('slide count', 7, {min: 0, max: 99});
+  const colorIncrement = Math.floor(255 / (slideCount + 1));
+  return (
+    <amp-base-carousel id="my-carousel" width={width} height={height}>
+      {Array.from({length: slideCount}, (x, i) => {
+        const v = colorIncrement * (i + 1);
+        return (
+          <div
+            style={{
+              backgroundColor: `rgb(${v}, 100, 100)`,
+              border: 'solid white 1px',
+              width: '100%',
+              height: '100%',
+            }}
+          ></div>
+        );
+      })}
+      <button slot="next-arrow">Next</button>
+      <button slot="prev-arrow">Prev</button>
+    </amp-base-carousel>
+  );
+};
+
 Default.storyName = 'default';
diff --git a/extensions/amp-base-carousel/1.0/storybook/Basic.js b/extensions/amp-base-carousel/1.0/storybook/Basic.js
@@ -170,6 +170,7 @@ export const provideArrows = () => {
     color: 'white',
     width: '30px',
     height: '30px',
+    padding: '1px 6px',
   };
   const MyButton = (props) => {
     const {children} = props;
@@ -184,8 +185,8 @@ export const provideArrows = () => {
       controls={controls}
       style={{width, height}}
       outsetArrows={outsetArrows}
-      arrowPrev={<MyButton>←</MyButton>}
-      arrowNext={<MyButton>→</MyButton>}
+      arrowPrevAs={(props) => <MyButton {...props}>←</MyButton>}
+      arrowNextAs={(props) => <MyButton {...props}>→</MyButton>}
     >
       {['lightcoral', 'peachpuff', 'lavender'].map((color) => (
         <div style={{backgroundColor: color, width, height}}></div>

diff --git a/extensions/amp-base-carousel/1.0/test/test-base-carousel.js b/extensions/amp-base-carousel/1.0/test/test-base-carousel.js
@@ -41,19 +41,27 @@ describes.sandboxed('BaseCarousel preact component', {}, () => {
   });
 
   it('should render custom Arrows when given', () => {
-    const arrowPrev = <div class="my-custom-arrow-prev">left</div>;
-    const arrowNext = <div class="my-custom-arrow-next">right</div>;
+    const arrowPrev = (props) => (
+      <div {...props} className="my-custom-arrow-prev">
+        left
+      </div>
+    );
+    const arrowNext = (props) => (
+      <div {...props} className="my-custom-arrow-next">
+        right
+      </div>
+    );
     const wrapper = mount(
-      <BaseCarousel arrowPrev={arrowPrev} arrowNext={arrowNext}>
+      <BaseCarousel arrowPrevAs={arrowPrev} arrowNextAs={arrowNext}>
         <div>slide 1</div>
         <div>slide 2</div>
         <div>slide 3</div>
       </BaseCarousel>
     );
     const arrows = wrapper.find('Arrow');
     expect(arrows).to.have.lengthOf(2);
-    expect(arrows.first().props().customArrow).to.equal(arrowPrev);
-    expect(arrows.last().props().customArrow).to.equal(arrowNext);
+    expect(arrows.first().props().as).to.equal(arrowPrev);
+    expect(arrows.last().props().as).to.equal(arrowNext);
   });
 
   it('should not loop by default', () => {

diff --git a/extensions/amp-stream-gallery/1.0/amp-stream-gallery.js b/extensions/amp-stream-gallery/1.0/amp-stream-gallery.js
@@ -21,6 +21,7 @@ import {CSS as GALLERY_CSS} from './stream-gallery.jss';
 import {PreactBaseElement} from '../../../src/preact/base-element';
 import {Services} from '../../../src/services';
 import {StreamGallery} from './stream-gallery';
+import {cloneElement} from '../../../src/preact';
 import {createCustomEvent} from '../../../src/event-helper';
 import {dict} from '../../../src/core/types/object';
 import {dispatchCustomEvent} from '../../../src/dom';
@@ -61,6 +62,17 @@ class AmpStreamGallery extends PreactBaseElement {
     );
     return super.isLayoutSupported(layout);
   }
+
+  /** @override */
+  updatePropsForRendering(props) {
+    const {arrowPrev, arrowNext} = props;
+    if (arrowPrev) {
+      props['arrowPrevAs'] = (props) => cloneElement(arrowPrev, props);
+    }
+    if (arrowNext) {
+      props['arrowNextAs'] = (props) => cloneElement(arrowNext, props);
+    }
+  }
 }
 
 /**

diff --git a/extensions/amp-stream-gallery/1.0/storybook/Basic.amp.js b/extensions/amp-stream-gallery/1.0/storybook/Basic.amp.js
@@ -91,4 +91,30 @@ export const Default = () => {
   );
 };
 
+export const customArrows = () => {
+  const width = number('width', 400);
+  const height = number('height', 200);
+  const slideCount = number('slide count', 7, {min: 0, max: 99});
+  const colorIncrement = Math.floor(255 / (slideCount + 1));
+  return (
+    <amp-stream-gallery max-visible-count={3} width={width} height={height}>
+      {Array.from({length: slideCount}, (x, i) => {
+        const v = colorIncrement * (i + 1);
+        return (
+          <div
+            style={{
+              backgroundColor: `rgb(${v}, 100, 100)`,
+              border: 'solid white 1px',
+              width: '100%',
+              height: '100%',
+            }}
+          ></div>
+        );
+      })}
+      <button slot="next-arrow">Next</button>
+      <button slot="prev-arrow">Prev</button>
+    </amp-stream-gallery>
+  );
+};
+
 Default.storyName = 'default';