✨ [bento][amp-sidebar] Preact version of amp-sidebar by krdwan · Pull Request #31479 · ampproject/amphtml

krdwan · 2020-12-07T20:45:08Z

First draft of Preact amp-sidebar component for initial review

12/11 - Update

Added unit tests -

imperative API
click to open, close, toggle
click mask to close
left and right sides
animations

Updated storybook

removed background content
add buttons to open, close, toggle to within sidebar

Added type definition file

dvoytenko

This looks pretty much there. No significant comments on structure.

extensions/amp-sidebar/1.0/sidebar.js

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

dvoytenko

Looks good, but one thing is not quite clear.

extensions/amp-sidebar/1.0/sidebar.js

caroqliu

Left a couple comments/questions but LGTM for the foundation. I'm curious what the plan is for:

checking/warning if the component is a direct descendent of <body>
responsive toolbar feature
future functionality for nested menus that interop with sidebar
bonus: if we were to add support for display locking, what would that look like? we could no longer conditionally render the markup

Would also be nice for the storybook to have a scrollable sample too.

caroqliu · 2020-12-11T19:21:22Z

extensions/amp-sidebar/1.0/sidebar.js

+ * @return {{current: T}}
+ * @template T
+ */
+function useValueRef(current) {


I think lightbox also defines useValueRef, do you want to move it to preact/index or preact/utils instead?

At this very initial stage, I'd like to keep it separate as I think a lot may change during the rest of the sidebar implementation. But I've added an item to the tracker to #31366 to look at shared items between this and lightbox. We have also talked about creating a common component that does the animations.

extensions/amp-sidebar/1.0/sidebar.js

caroqliu · 2020-12-11T19:25:30Z

extensions/amp-sidebar/1.0/sidebar.js

+  // To open, we mount and render the contents (invisible), then animate the display (visible).
+  // To close, it's the reverse.
+  // `mounted` mounts the component. `opened` plays the animation.
+  const initialOpen = false;


If this is always false, do we need a named constant for the two times it's used?

Sure updated!

caroqliu · 2020-12-11T19:27:04Z

extensions/amp-sidebar/1.0/sidebar.js

+  }, [onBeforeOpenRef]);
+  const close = useCallback(() => setOpened(false), []);
+  const toggle = useCallback(() => (opened ? close() : open()), [
+    opened,


Would it be helpful to have an openedRef so this callback value does not have to be redefined every time the state changes?

Thanks, will address this as a follow up once we get further into the sidebar implementation.

caroqliu · 2020-12-11T19:29:14Z

extensions/amp-sidebar/1.0/sidebar.js

+  // `mounted` mounts the component. `opened` plays the animation.
+  const initialOpen = false;
+  const [mounted, setMounted] = useState(initialOpen);
+  const [opened, setOpened] = useState(initialOpen);


nit: Should this follow [visible, setVisible] like lightbox? the meaning of "open" could get a bit overloaded here

I personally prefer opened since the sidebar docs all refer to the sidebar as being opened / closed. 🙂

caroqliu · 2020-12-11T19:30:57Z

extensions/amp-sidebar/1.0/sidebar.js

+          : ANIMATION_KEYFRAMES_SLIDE_IN_RIGHT,
+        {
+          duration: ANIMATION_DURATION,
+          fill: 'both',


FMI: Why are some of these style objects given constant doc level names and others inlined here?

It is a tiny bit wonky, but my thought process was -

fill and direction are not configurable as they are fundamentally needed for the way the animation works.
duration and ease-in can be configured to make the animation slower / faster, curvier, and it still works, so thought it made sense to make as a constant.

I'm not married to it either way so let me know if you feel strongly about it.

caroqliu · 2020-12-11T19:33:28Z

extensions/amp-sidebar/1.0/sidebar.js

+            side === 'left' ? classes.left : classes.right
+          }`}
+          role="menu"
+          tabindex="-1"


Isn't a <div> not focusable by default?

Also, shouldn't it be focusable so it can listen for Esc to close?

The only ways to close that have been implemented so far are the imperative API and click on the mask. I have an action item in the tracker: #31366 for handling focus, so I'll follow up at that point :). This may be updated.

caroqliu · 2020-12-11T19:34:17Z

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

+import {createUseStyles} from 'react-jss';
+
+const sidebarClass = {
+  position: 'fixed !important',


FMI: Why do we need these !importants?

Mostly just so the user cannot over overwrite by accident (or on purpose). The way the sidebar animates in and is positioned on the viewport is all critically dependent on position: fixed.

caroqliu · 2020-12-11T19:36:24Z

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

+  return (
+    <>
+      <Sidebar ref={ref} {...props}>
+        <div style={{margin: 8}}>


Is this margin necessary?

It's not needed per se, but I think it looks infinitely better with the margin. 😂

Are you opposed to it?

This is storybook only, right?

Right, this is just in the storybook.

krdwan added 2 commits December 7, 2020 15:34

Sidebar with storybook

962c991

Sidebar preact component

e71e1cd

krdwan requested a review from dvoytenko December 7, 2020 20:45

krdwan added 2 commits December 7, 2020 15:49

Use ternary operator

e87db9f

Update animation cancel callbacks

1b53aab

dvoytenko reviewed Dec 7, 2020

View reviewed changes

Update per review comments

491b2fa

krdwan requested a review from dvoytenko December 7, 2020 22:58

Update sidebar animations

877a718

dvoytenko suggested changes Dec 9, 2020

View reviewed changes

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

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

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

Updates per code review

2a4e3fa

dvoytenko approved these changes Dec 9, 2020

View reviewed changes

Add tests update storybook for sidebar

98b8b4a

krdwan marked this pull request as ready for review December 11, 2020 17:37

caroqliu self-requested a review December 11, 2020 17:42

dvoytenko approved these changes Dec 11, 2020

View reviewed changes

caroqliu approved these changes Dec 11, 2020

View reviewed changes

Review comments

57c6d1b

krdwan merged commit 32e092f into ampproject:master Dec 11, 2020

krdwan changed the title ~~✨ [bento] Preact version of amp-sidebar~~ ✨ [bento][amp-sidebar] Preact version of amp-sidebar Dec 11, 2020

ampprojectbot added the PR use: In Beta / Experimental label Dec 16, 2020

krdwan mentioned this pull request Dec 16, 2020

[bento][amp-sidebar] Initial files for bento sidebar #31324

Closed

6 tasks

ampprojectbot added the PR use: In Stable label Jan 5, 2021

ampprojectbot added the PR use: In LTS label Jan 13, 2021

Conversation

krdwan commented Dec 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dvoytenko left a comment

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!

dvoytenko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

caroqliu left a comment

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

krdwan commented Dec 7, 2020 •

edited

Loading