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

[krdwan]

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)

[krdwan]

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

[caroqliu]

Suggested change

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

[krdwan]

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).

[caroqliu]

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

[krdwan]

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 :)

[caroqliu]

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.

[krdwan]

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]

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]

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

[alanorozco]

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.

[krdwan]

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!

[alanorozco]

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, ...};

[alanorozco]

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

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

Line 58 in 343229a

zIndex: 2147483646,

[krdwan]

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

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

[krdwan]

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]

Why not apply this class to ContainWrapper?

[krdwan]

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

[caroqliu]

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

[jridgewell]

LGTM

[jridgewell]

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

[krdwan]

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">