✨ [bento][amp-sidebar] Add nav toolbar component on Preact side only by krdwan · Pull Request #33244 · ampproject/amphtml

krdwan · 2021-03-12T20:39:38Z

3/17
Updated the design to listen on the MediaQueryList returned from the window.matchMedia call instead of listening on the window (per review comments).

Amp-sidebar Tracker: #31366

Preact side of nav toolbar feature as discussed in Bento standup on 3/11
Design doc: Design Doc

To do:

update type file
add tests
update storybook
follow up PR with amp side (added to tracker for tracking)

extensions/amp-sidebar/1.0/component.js

krdwan · 2021-03-12T20:47:47Z

extensions/amp-sidebar/1.0/component.js

+  toolbar,
+  'toolbar-target': toolbarTarget,
+  children,
+  ...rest


Will add an as prop when I do the AMP mode PR

extensions/amp-sidebar/1.0/component.js

caroqliu · 2021-03-18T13:42:58Z

extensions/amp-sidebar/1.0/component.js

+
+  return (
+    <>
+      <nav ref={ref} toolbar={toolbar} toolbar-target={toolbarTarget} {...rest}>


Suggested change

<nav ref={ref} toolbar={toolbar} toolbar-target={toolbarTarget} {...rest}>

<nav ref={ref} toolbar={toolbar} toolbarTarget={toolbarTarget} {...rest}>

So I've updated the prop value from toolbar-target to toolbarTarget. This is the rendered output and should stay as toolbar-target (this is the actual HTML attribute) that gets rendered out. (Should be in kebab case).

Why is it important for it to be rendered out in kebab case?

Html attributes are in kebab case? So the output would look like this:

<nav toolbar="(max-width: 500px)" toolbar-target="toolbar-target">...

whereas if we use camel case here it would output like this:

<nav toolbar="(max-width: 500px)" toolbartarget="toolbar-target">...

Also the first one is aligned with what our 0.1 code would look like :)

extensions/amp-sidebar/1.0/component.type.js

caroqliu · 2021-03-18T13:52:01Z

extensions/amp-sidebar/1.0/storybook/Basic.js

+
+  return (
+    <main>
+      <SidebarWithActions
+        side={side}
+        style={{color: foregroundColor, backgroundColor}}
+        backdropStyle={{backgroundColor: backdropColor}}
+      >
+        <SidebarToolbar toolbar={toolbarMedia} toolbar-target="toolbar-target">
+          <ul>
+            <li>Toolbar Item 1</li>
+            <li>Toolbar Item 2</li>
+          </ul>
+        </SidebarToolbar>
+      </SidebarWithActions>
+      <div id="toolbar-target"></div>
+    </main>
+  );


This is the toolbarTarget as ref idea:

Suggested change

return (

<main>

<SidebarWithActions

side={side}

style={{color: foregroundColor, backgroundColor}}

backdropStyle={{backgroundColor: backdropColor}}

>

<SidebarToolbar toolbar={toolbarMedia} toolbar-target="toolbar-target">

<ul>

<li>Toolbar Item 1</li>

<li>Toolbar Item 2</li>

</ul>

</SidebarToolbar>

</SidebarWithActions>

<div id="toolbar-target"></div>

</main>

);

const targetRef = useRef(null);

return (

<main>

<SidebarWithActions

side={side}

style={{color: foregroundColor, backgroundColor}}

backdropStyle={{backgroundColor: backdropColor}}

>

<SidebarToolbar toolbar={toolbarMedia} toolbarTarget={targetRef}>

<ul>

<li>Toolbar Item 1</li>

<li>Toolbar Item 2</li>

</ul>

</SidebarToolbar>

</SidebarWithActions>

<div ref={targetRef}></div>

</main>

);

Then your createPortal line would look like:
{matches && toolbarTarget.current && createPortal(children, toolbarTarget.current)}

I'm actually not sure if it's advisable to pass a ref like this, and usually indicates that some outer context may be helpful to manage things more explicitly... But the current approach of querying the document doesn't allow for updating the portalled parent i.e. if the same id gets reassigned to a different element between renders.

I see, I'm open to it either way. Let's see if anyone else feels strongly about it. If not, we can table it for now and explore further as a follow up.

krdwan · 2021-03-18T16:41:40Z

extensions/amp-sidebar/1.0/sidebar-animations-hook.js

+    // sidebarElement and backdropElement would not be rendered when
+    // mounted is false.  This is no longer the case.
+    if (!mounted || !sidebarElement || !backdropElement || !side) {
      return;


Added a check for mounted here. Per the comment, we don't want animations running in an unmounted state. Previously checking sidebarElement and backdropElement refs were enough since they were never rendered in unmounted state.

krdwan · 2021-03-18T16:42:27Z

extensions/amp-sidebar/1.0/test/test-component.js

+  const isOpened = (sidebarElement) => {
+    return !sidebarElement.className.includes('unmounted');
+  };
+


How the unit tests check whether the sidebar is opened or not must be updated throughout the test.

extensions/amp-sidebar/1.0/component.js

alanorozco · 2021-03-18T16:57:10Z

extensions/amp-sidebar/1.0/component.js

+        className={objstr({
+          [backdropClassName]: backdropClassName,
+          [classes.backdrop]: true,
+          [classes.defaultBackdropStyles]: true,
+        })}


nit: Place constant true values at the beginning

Suggested change

className={objstr({

[backdropClassName]: backdropClassName,

[classes.backdrop]: true,

[classes.defaultBackdropStyles]: true,

})}

className={objstr({

[classes.backdrop]: true,

[classes.defaultBackdropStyles]: true,

[backdropClassName]: backdropClassName,

})}

Also: if backdrop and defaultBackdropStyles don't occur independently, merge them.

Thanks! Updated to put true values at the top. Regarding the backdrop and defaultBackdropStyles, the reason these are split out is because the default ones are meant to be overridable by the user, whereas the backdrop class has styles that are built in to the sidebar. I sort of like having them split out to emphasize the distinction, but let me know if you feel strongly about it!

I don't know if our JSS transform supports this (try it?) but how about exposing a single classname, but split them in definition anyhow?

// these can overridden const defaultBackdropStyles = { ... }; const backdrop = { // x: "... !important", ...defaultBackdropStyles, }; export {..., backdrop, ...};

also, in backdrop: shouldn't this be !important?

amphtml/extensions/amp-sidebar/1.0/component.jss.js

Line 58 in 343229a

zIndex: 2147483646,

Hmmm.. it doesn't seem to like it. See below -

You're right, I've moved zIndex to be configurable!

I went ahead and merged for now, but let me know if you think there should be any other changes and I can follow up!

caroqliu · 2021-03-22T14:35:16Z

extensions/amp-sidebar/1.0/component.js

-        </div>
-      </>
-    )
+    <div className={objstr({[classes.unmounted]: !mounted})} part="wrapper">


Why not apply this class to ContainWrapper?

ContainWrapper does not contain the backdrop so I wanted the class to apply to everything. Is there something else I'm missing?

caroqliu · 2021-03-22T14:37:26Z

extensions/amp-sidebar/1.0/component.js

+
+  return (
+    <>
+      <nav ref={ref} toolbar={toolbar} toolbar-target={toolbarTarget} {...rest}>


Why is it important for it to be rendered out in kebab case?

jridgewell

LGTM

extensions/amp-sidebar/1.0/component.js

jridgewell · 2021-03-23T00:32:32Z

extensions/amp-sidebar/1.0/component.js

+      return;
+    }
+
+    const mediaQueryList = window.matchMedia(toolbar);


Can we rename toolbar to be more descriptive? It's a media query, right?

That works, I've updated it to mediaQueryProp internally (within the component). I think we should keep the prop name toolbar externally since it matches with the existing API for 0.1. (In 0.1 the user would specify a toolbar as such:)

<nav toolbar="(media-query)" toolbar-target="elementID">

krdwan commented Mar 12, 2021

View reviewed changes

extensions/amp-sidebar/1.0/component.js Outdated Show resolved Hide resolved

krdwan commented Mar 12, 2021

View reviewed changes

extensions/amp-sidebar/1.0/component.js Outdated Show resolved Hide resolved

krdwan commented Mar 12, 2021

View reviewed changes

extensions/amp-sidebar/1.0/component.js Outdated Show resolved Hide resolved

krdwan requested review from caroqliu, dvoytenko and jridgewell March 12, 2021 20:49

krdwan marked this pull request as ready for review March 12, 2021 21:04

caroqliu reviewed Mar 15, 2021

View reviewed changes

krdwan mentioned this pull request Mar 15, 2021

[amp-sidebar:1.0] Bento tracking issue #31366

Open

23 tasks

krdwan requested a review from alanorozco March 16, 2021 14:00

jridgewell reviewed Mar 17, 2021

View reviewed changes

krdwan force-pushed the sidebar-nav-toolbar branch from 896e023 to f62f164 Compare March 17, 2021 19:43

krdwan requested review from caroqliu and jridgewell March 17, 2021 20:17

krdwan force-pushed the sidebar-nav-toolbar branch from f9bb691 to 170c623 Compare March 17, 2021 20:25

caroqliu reviewed Mar 18, 2021

View reviewed changes

krdwan commented Mar 18, 2021

View reviewed changes

alanorozco approved these changes Mar 18, 2021

View reviewed changes

krdwan requested a review from caroqliu March 18, 2021 19:10

krdwan force-pushed the sidebar-nav-toolbar branch from 70027f5 to 0906f71 Compare March 19, 2021 14:51

caroqliu approved these changes Mar 22, 2021

View reviewed changes

jridgewell approved these changes Mar 23, 2021

View reviewed changes

krdwan added 6 commits March 24, 2021 15:28

Add nav toolbar component

37b9352

Review comments

36f58c3

Update design to listen on mediaQueryList

dae1c49

Add obj-str

018e767

Update type definitions

1cc81a3

Add storybook

e5677d9

krdwan added 8 commits March 24, 2021 15:28

Objstr update

bb3052c

Update tests to work with always rendered children

ace57f1

Review comments toolbarTarget and side

113b651

Add tests for toolbar

1c6e537

Rearrange classes so true ones are at top

ad6af6c

Issues and tests updated for additional container element on amp side

f2c9090

Update toolbar prop name to be more descriptive

5cb19b8

Update CSS per review comments

8602490

krdwan force-pushed the sidebar-nav-toolbar branch from 343229a to 8602490 Compare March 24, 2021 19:29

Fix z-index with amp get-zindex fix

73ce7f9

krdwan merged commit 1bd7416 into ampproject:master Mar 24, 2021

caroqliu mentioned this pull request Mar 29, 2021

Bento: add container to the lightbox 1.0 #33488

Merged

ampprojectbot added the PR use: In Beta / Experimental label Mar 30, 2021

ampprojectbot added PR use: In Stable PR use: In LTS labels Apr 6, 2021

	<nav ref={ref} toolbar={toolbar} toolbar-target={toolbarTarget} {...rest}>
	<nav ref={ref} toolbar={toolbar} toolbarTarget={toolbarTarget} {...rest}>

Conversation

krdwan commented Mar 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krdwan Mar 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jridgewell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

krdwan commented Mar 12, 2021 •

edited

Loading

krdwan Mar 18, 2021 •

edited

Loading