Skip to content

✨ [bento][amp-sidebar] Toolbar design for AMP side for amp-sidebar#34368

Merged
krdwan merged 5 commits intoampproject:mainfrom
krdwan:sidebar-ssr-amp
Jun 4, 2021
Merged

✨ [bento][amp-sidebar] Toolbar design for AMP side for amp-sidebar#34368
krdwan merged 5 commits intoampproject:mainfrom
krdwan:sidebar-ssr-amp

Conversation

@krdwan
Copy link
Copy Markdown
Contributor

@krdwan krdwan commented May 13, 2021

Sidebar Tracker: #31366

Toolbar design amp side for amp-sidebar

How does this work?

  1. The preact Toolbar will be leveraged in both cases. It has been refactored into ToolbarHelper with a wrapper preact component to be used each in amp and preact modes.
  2. Each of the wrapper components pass down context to indicate whether we are in amp or preact mode. When in amp mode, there is also an actual Toolbar domElement which is passed via context.
  3. When in preact mode, the behavior is relatively unchanged.
  4. When in amp mode, the base-element updates the children prop to be passed into preact by appending amp version of the Toolbar (the amp wrapper from 1 above). This amp version of the toolbar takes in the actual domElement and clones it into the toolbar target. It also does not render anything in place. So the children prop will include slot and a number of these amp version Toolbars.

What is Toolbar?

Toolbar takes a nav element from within the Sidebar and clones itself as a child onto an arbitrary element somewhere on the page. (Which I'll refer to as Toolbar Target). An additional style element is also appended to the Toolbar Target to conditionally display the element when a mediaQuery is true.

Design doc: Doc
Preact Side toolbar: #34158

to do: tests

static deferredMount(unusedElement) {
return false;
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allows component render before Sidebar is opened. This is needed because Toolbar Target should be rendered on page load, regardless of Sidebar's open / close status.

@krdwan krdwan requested review from caroqliu and jridgewell May 13, 2021 18:51
@krdwan krdwan marked this pull request as ready for review May 13, 2021 18:51
Copy link
Copy Markdown
Contributor

@caroqliu caroqliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great start with this. One important point - the Preact layer should really not be concerned with what environment it is run in. I know you mentioned wanting to keep the props namespace clean, but it may be helpful to provide configurable props that the AMP layer can use as opposed to fit the Preact layer to the AMP layer specifically. And the use of the context does not simplify the mental load of adding more props IMO.

Made a recommendation in line but feel free to ping if you want to discuss further.

@krdwan krdwan force-pushed the sidebar-ssr-amp branch from 2b67abd to 5f9591f Compare June 3, 2021 16:17
@krdwan krdwan requested a review from caroqliu June 3, 2021 17:07
@krdwan krdwan enabled auto-merge (squash) June 4, 2021 16:53
@krdwan krdwan merged commit 97a0ed8 into ampproject:main Jun 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants