✨ [bento][amp-sidebar] Add nav toolbar component on Preact side only (#…

…33244) * Add nav toolbar component * Review comments * Update design to listen on mediaQueryList * Add obj-str * Update type definitions * Add storybook * Objstr update * Update tests to work with always rendered children * Review comments toolbarTarget and side * Add tests for toolbar * Rearrange classes so true ones are at top * Issues and tests updated for additional container element on amp side * Update toolbar prop name to be more descriptive * Update CSS per review comments * Fix z-index with amp get-zindex fix
ampproject · Mar 24, 2021 · 1bd7416 · 1bd7416
1 parent 3e249e2
commit 1bd7416
Show file tree

Hide file tree

Showing 9 changed files with 570 additions and 269 deletions.
diff --git a/css/Z_INDEX.md b/css/Z_INDEX.md
diff --git a/extensions/amp-sidebar/1.0/amp-sidebar.css b/extensions/amp-sidebar/1.0/amp-sidebar.css
@@ -37,6 +37,7 @@ amp-sidebar {
 }
 
 amp-sidebar::part(c),
+amp-sidebar::part(wrapper),
 amp-sidebar::part(sidebar) {
   /* Inherit values specified on amp-sidebar */
   color: inherit;

diff --git a/extensions/amp-sidebar/1.0/component.js b/extensions/amp-sidebar/1.0/component.js
@@ -18,7 +18,7 @@ import * as Preact from '../../../src/preact';
 import {ContainWrapper, useValueRef} from '../../../src/preact/component';
 import {Keys} from '../../../src/utils/key-codes';
 import {Side} from './sidebar-config';
-import {forwardRef} from '../../../src/preact/compat';
+import {createPortal, forwardRef} from '../../../src/preact/compat';
 import {isRTL} from '../../../src/dom';
 import {
   useCallback,
@@ -30,9 +30,10 @@ import {
 } from '../../../src/preact';
 import {useSidebarAnimation} from './sidebar-animations-hook';
 import {useStyles} from './component.jss';
+import objstr from 'obj-str';
 
 /**
- * @param {!SidebarDef.Props} props
+ * @param {!SidebarDef.SidebarProps} props
  * @param {{current: (!SidebarDef.SidebarApi|null)}} ref
  * @return {PreactDef.Renderable}
  */
@@ -103,6 +104,7 @@ function SidebarWithRef(
   }, [side, mounted]);
 
   useSidebarAnimation(
+    mounted,
     opened,
     onAfterClose,
     side,
@@ -135,42 +137,98 @@ function SidebarWithRef(
   }, [opened, close]);
 
   return (
-    mounted && (
-      <>
-        <ContainWrapper
-          as={Comp}
-          ref={sidebarRef}
-          size={false}
-          layout={true}
-          paint={true}
-          part="sidebar"
-          wrapperClassName={`${classes.sidebar} ${
-            classes.defaultSidebarStyles
-          } ${side === Side.LEFT ? classes.left : classes.right}`}
-          role="menu"
-          tabindex="-1"
-          hidden={!side}
-          {...rest}
-        >
-          {children}
-        </ContainWrapper>
-        <div
-          ref={backdropRef}
-          onClick={() => close()}
-          part="backdrop"
-          style={backdropStyle}
-          className={`${backdropClassName ?? ''} ${classes.backdrop} ${
-            classes.defaultBackdropStyles
-          }`}
-          hidden={!side}
-        >
-          <div className={classes.backdropOverscrollBlocker}></div>
-        </div>
-      </>
-    )
+    <div className={objstr({[classes.unmounted]: !mounted})} part="wrapper">
+      <ContainWrapper
+        as={Comp}
+        ref={sidebarRef}
+        size={false}
+        layout={true}
+        paint={true}
+        part="sidebar"
+        wrapperClassName={objstr({
+          [classes.sidebar]: true,
+          [classes.defaultSidebarStyles]: true,
+          [classes.left]: side === Side.LEFT,
+          [classes.right]: side !== Side.LEFT,
+        })}
+        role="menu"
+        tabindex="-1"
+        hidden={!side}
+        {...rest}
+      >
+        {children}
+      </ContainWrapper>
+      <div
+        ref={backdropRef}
+        onClick={() => close()}
+        part="backdrop"
+        style={backdropStyle}
+        className={objstr({
+          [classes.backdrop]: true,
+          [classes.defaultBackdropStyles]: true,
+          [backdropClassName]: backdropClassName,
+        })}
+        hidden={!side}
+      >
+        <div className={classes.backdropOverscrollBlocker}></div>
+      </div>
+    </div>
   );
 }
 
 const Sidebar = forwardRef(SidebarWithRef);
 Sidebar.displayName = 'Sidebar'; // Make findable for tests.
 export {Sidebar};
+
+/**
+ * @param {!SidebarDef.SidebarToolbarProps} props
+ * @return {PreactDef.Renderable}
+ */
+export function SidebarToolbar({
+  toolbar: mediaQueryProp,
+  toolbarTarget,
+  children,
+  ...rest
+}) {
+  const ref = useRef();
+  const [matches, setMatches] = useState(false);
+  const [targetEl, setTargetEl] = useState(null);
+
+  useEffect(() => {
+    const window = ref.current?.ownerDocument?.defaultView;
+    if (!window) {
+      return;
+    }
+
+    const mediaQueryList = window.matchMedia(mediaQueryProp);
+    const updateMatches = () => setMatches(mediaQueryList.matches);
+    mediaQueryList.addEventListener('change', updateMatches);
+    setMatches(mediaQueryList.matches);
+    return () => mediaQueryList.removeEventListener('change', updateMatches);
+  }, [mediaQueryProp]);
+
+  useEffect(() => {
+    const document = ref.current?.ownerDocument;
+    if (!document) {
+      return;
+    }
+
+    const selector = `#${CSS.escape(toolbarTarget)}`;
+    const newTargetEl = document.querySelector(selector);
+    setTargetEl(newTargetEl);
+  }, [toolbarTarget]);
+
+  return (
+    <>
+      <nav
+        ref={ref}
+        toolbar={mediaQueryProp}
+        toolbar-target={toolbarTarget}
+        {...rest}
+      >
+        {children}
+      </nav>
+      {matches && targetEl && createPortal(children, targetEl)}
+    </>
+  );
+}
diff --git a/extensions/amp-sidebar/1.0/component.jss.js b/extensions/amp-sidebar/1.0/component.jss.js
@@ -55,12 +55,12 @@ const backdrop = {
   /* Prevent someone from making this a full-sceen image */
   backgroundImage: 'none !important',
   overscrollBehavior: 'none !important',
-  zIndex: 2147483646,
 };
 
 // User overridable styles
 const defaultBackdropStyles = {
   backgroundColor: 'rgba(0, 0, 0, 0.5)',
+  zIndex: 2147483646,
 };
 
 //TODO(#32400): See PR description.  This is a workaround dependent on a
@@ -70,6 +70,10 @@ const backdropOverscrollBlocker = {
   width: '0 !important',
 };
 
+const unmounted = {
+  display: 'none',
+};
+
 const JSS = {
   sidebar,
   defaultSidebarStyles,
@@ -78,6 +82,7 @@ const JSS = {
   backdrop,
   defaultBackdropStyles,
   backdropOverscrollBlocker,
+  unmounted,
 };
 
 // useStyles gets replaced for AMP builds via `babel-plugin-transform-jss`.

diff --git a/extensions/amp-sidebar/1.0/component.type.js b/extensions/amp-sidebar/1.0/component.type.js
@@ -30,7 +30,16 @@ var SidebarDef = {};
  *   children: (?PreactDef.Renderable|undefined),
  * }}
  */
-SidebarDef.Props;
+SidebarDef.SidebarProps;
+
+/**
+ * @typedef {{
+ *   toolbar: (string|undefined),
+ *   toolbarTarget: (string|undefined),
+ *   children: (?PreactDef.Renderable|undefined),
+ * }}
+ */
+SidebarDef.SidebarToolbarProps;
 
 /** @interface */
 SidebarDef.SidebarApi = class {

diff --git a/extensions/amp-sidebar/1.0/sidebar-animations-hook.js b/extensions/amp-sidebar/1.0/sidebar-animations-hook.js
@@ -47,6 +47,7 @@ function safelySetStyles(element, styles) {
 }
 
 /**
+ * @param {boolean} mounted
  * @param {boolean} opened
  * @param {{current: function|undefined}} onAfterClose
  * @param {string} side
@@ -55,6 +56,7 @@ function safelySetStyles(element, styles) {
  * @param {function} setMounted
  */
 export function useSidebarAnimation(
+  mounted,
   opened,
   onAfterClose,
   side,
@@ -71,7 +73,10 @@ export function useSidebarAnimation(
     const backdropElement = backdropRef.current;
     // The component might start in a state where `side` is not known
     // This effect must be restarted when the `side` becomes known
-    if (!sidebarElement || !backdropElement || !side) {
+    // Must also check mounted as of #33244.  This is because previously
+    // sidebarElement and backdropElement would not be rendered when
+    // mounted is false.  This is no longer the case.
+    if (!mounted || !sidebarElement || !backdropElement || !side) {
       return;
     }
 
@@ -175,5 +180,13 @@ export function useSidebarAnimation(
       backdropAnimationRef.current = backdropAnimation;
       currentlyAnimatingRef.current = true;
     }
-  }, [opened, onAfterCloseRef, side, sidebarRef, backdropRef, setMounted]);
+  }, [
+    mounted,
+    opened,
+    onAfterCloseRef,
+    side,
+    sidebarRef,
+    backdropRef,
+    setMounted,
+  ]);
 }
diff --git a/extensions/amp-sidebar/1.0/storybook/Basic.js b/extensions/amp-sidebar/1.0/storybook/Basic.js
@@ -15,8 +15,8 @@
  */
 
 import * as Preact from '../../../../src/preact';
-import {Sidebar} from '../component';
-import {boolean, color, select, withKnobs} from '@storybook/addon-knobs';
+import {Sidebar, SidebarToolbar} from '../component';
+import {boolean, color, select, text, withKnobs} from '@storybook/addon-knobs';
 import {withA11y} from '@storybook/addon-a11y';
 
 export default {
@@ -80,6 +80,33 @@ export const _default = () => {
   );
 };
 
+export const toolbar = () => {
+  const sideConfigurations = ['left', 'right', undefined];
+  const side = select('side', sideConfigurations, sideConfigurations[0]);
+  const toolbarMedia = text('toolbar media', '(max-width: 500px)');
+  const foregroundColor = color('color');
+  const backgroundColor = color('background');
+  const backdropColor = color('backdrop color');
+
+  return (
+    <main>
+      <SidebarWithActions
+        side={side}
+        style={{color: foregroundColor, backgroundColor}}
+        backdropStyle={{backgroundColor: backdropColor}}
+      >
+        <SidebarToolbar toolbar={toolbarMedia} toolbarTarget="toolbar-target">
+          <ul>
+            <li>Toolbar Item 1</li>
+            <li>Toolbar Item 2</li>
+          </ul>
+        </SidebarToolbar>
+      </SidebarWithActions>
+      <div id="toolbar-target"></div>
+    </main>
+  );
+};
+
 export const scroll = () => {
   const sideConfigurations = ['left', 'right', undefined];
   const side = select('side', sideConfigurations, sideConfigurations[0]);