[Dialog][AlertDialog] Add portal part #936

jjenzz · 2021-10-26T12:01:51Z

Easier to review with hidden whitespace changes

First instalment for #760

Vlad is struggling with the inability to portal dialogs into a custom container on the new primitives website. This should unblock him while we find time to roll this out to other primitives.

Notes for docs

New Portal parts for Dialog and AlertDialog
Component parts will not portal if not wrapped in portal part
Portal part doesn't render a DOM node, need to apply position and z-index to Overlay and Content

Part has container prop which must be HTMLElement so document example of this

const [container, setContainer] = React.useState<HTMLDivElement | null>(null);
return (
    <>
      <Dialog.Portal container={container} />
      <div ref={setContainer} />
    </>
);

packages/react/dialog/src/Dialog.tsx

jjenzz · 2021-10-26T12:15:05Z

packages/react/dialog/src/Dialog.tsx

-          />
-        </RemoveScroll>
-      </Portal>
+      <RemoveScroll as={Slot} allowPinchZoom={allowPinchZoom}>


Realised we could as={Slot} this to reduce the number of divs rendered. I tested this on desktop and iOS and scroll is still prevented fine. The allowPinchZoom story also still works.

This is neat too.

packages/react/portal/src/Portal.tsx

jjenzz · 2021-10-26T12:19:15Z

packages/react/portal/src/Portal.tsx

+
+type UnstablePortalElement = React.ElementRef<typeof Primitive.div>;
+interface UnstablePortalProps extends PrimitiveDivProps {
+  container?: HTMLElement | null;


I've made this expect the node instead of a ref because if we expect a ref we have to build in assumptions that mightn't be correct.

You'll see in the old Portal implementation that we assume the element exists on the page and that the ref will be available when we force a re-render which might not be the case... Perhaps the node they want to portal into is mounted at the same time as the Dialog.Content (if they're controlling it) 🤷‍♂️

This approach means we will get the correct container whenever it becomes available.

I'm a bit unsure about this because I remember that when we rejigged the portal implementation, we did try this but it broke some of our Portal chromatic stories around custom containers.

I'll add a story for this new UnstablePortal to test this theory. I would expect this version to work the same in Chromatic because our previous Portal was forcing a re-render for it to work and this approach will be doing the same.

~~Yeah, tested and it does behave differently... that's super strange, they're both forcing a re-render 🤔 Gonna look into this some more.~~

Oh no wait, I'm being a muppet... it obviously looked different cos the styles are removed haha. I've moved positioning into story and looks fine so let's see what Chromatic does when it finishes building.

UPDATE: both look the same here

jjenzz · 2021-10-26T12:53:53Z

packages/react/portal/src/Portal.tsx

+
+const UnstablePortal: React.FC<UnstablePortalProps> = (props) => {
+  const { container = globalThis?.document?.body, children } = props;
+  return container ? ReactDOM.createPortal(<>{children}</>, container) : null;


Portal wouldn't render a div anymore. This removes the confusion of applying zIndex to Dialog.Content and having the wrong stacking. Consumers can apply all layering CSS to Dialog.Content and Dialog.Overlay directly where they add their position: fixed styles and get the expected result.

I like the simplicity of this, but I think we've lost a big part of the overall solution though with regards to the layering. In my mind, I thought we would open up to provide a way to override our defaults, but right now it seems we just don't offer defaults anymore with this approach so the layering can very easily be wrong out of the box.

I thought we would open up to provide a way to override our defaults, but right now it seems we just don't offer defaults anymore with this approach so the layering can very easily be wrong out of the box.

It seemed to me that we needed to offer defaults for this that "worked out of the box" because people couldn't access this part to style it. We created a problem for ourselves by closing over the part that required default styles to solve, but this change removes that problem.

so the layering can very easily be wrong out of the box.

Now that the portal doesn't render anything, all of their position: fixed; zIndex: 100 styles that they apply to Dialog.Content (which is the more natural place to apply them) will work as intended.

We don't provide position: fixed styles for Dialog.Content so not sure why we would hard-code positioning and layering into portal either, especially when portals don't have to be on top of things.

I've had a chat with @jjenzz about this and I think I'm aligned with this now. @colmtuite might want to take a look at some point as this is a slight difference to what we were doing before though.

packages/react/dialog/src/Dialog.tsx

benoitgrelard · 2021-10-26T13:25:12Z

packages/react/portal/src/Portal.tsx

+
+type UnstablePortalElement = React.ElementRef<typeof Primitive.div>;
+interface UnstablePortalProps extends PrimitiveDivProps {
+  container?: HTMLElement | null;


I'm a bit unsure about this because I remember that when we rejigged the portal implementation, we did try this but it broke some of our Portal chromatic stories around custom containers.

benoitgrelard · 2021-10-26T13:26:24Z

packages/react/portal/src/Portal.tsx

+
+const UnstablePortal: React.FC<UnstablePortalProps> = (props) => {
+  const { container = globalThis?.document?.body, children } = props;
+  return container ? ReactDOM.createPortal(<>{children}</>, container) : null;


I like the simplicity of this, but I think we've lost a big part of the overall solution though with regards to the layering. In my mind, I thought we would open up to provide a way to override our defaults, but right now it seems we just don't offer defaults anymore with this approach so the layering can very easily be wrong out of the box.

benoitgrelard

I like the changes, this is definitely the right direction to go with all primitives I think.
Also, Dialog is a good one to have people try as it was probably the one with the most requests around this subject, so I'm looking forward to hear from users if we've addressed their concerns/issues.

csantos-nydig · 2021-11-10T15:35:43Z

packages/react/portal/src/Portal.tsx

+  return container ? ReactDOM.createPortal(<>{children}</>, container) : null;
+};
+
+Portal.displayName = UNSTABLE_PORTAL_NAME;


UnstablePortal.displayName = UNSTABLE_PORTAL_NAME;

😄

😅 good spot! Fancy creating a new PR?

jjenzz requested review from andy-hook and benoitgrelard as code owners October 26, 2021 12:01

jjenzz force-pushed the expose-portal-dialog branch 3 times, most recently from 4929a13 to b1f1361 Compare October 26, 2021 12:11

Add portal part to Dialog

171a975

jjenzz force-pushed the expose-portal-dialog branch from b1f1361 to 171a975 Compare October 26, 2021 12:12

jjenzz commented Oct 26, 2021

View reviewed changes

packages/react/dialog/src/Dialog.tsx Show resolved Hide resolved

jjenzz commented Oct 26, 2021

View reviewed changes

benoitgrelard reviewed Oct 26, 2021

View reviewed changes

packages/react/portal/src/Portal.tsx Outdated Show resolved Hide resolved

jjenzz commented Oct 26, 2021

View reviewed changes

jjenzz force-pushed the expose-portal-dialog branch from 764ea49 to 43edbe7 Compare October 26, 2021 12:58

Prevent portal from rendering a node

7c858d2

jjenzz force-pushed the expose-portal-dialog branch from 43edbe7 to 7c858d2 Compare October 26, 2021 13:00

jjenzz requested a review from benoitgrelard October 26, 2021 13:03

benoitgrelard reviewed Oct 26, 2021

View reviewed changes

UnstablePortal stories

762b204

benoitgrelard approved these changes Oct 26, 2021

View reviewed changes

jjenzz merged commit 8af7611 into main Oct 26, 2021

jjenzz deleted the expose-portal-dialog branch October 26, 2021 16:01

csantos-nydig reviewed Nov 10, 2021

View reviewed changes

csantos1113 mentioned this pull request Nov 19, 2021

[Portal] Fix UnstablePortal.displayName #988

Merged

[Dialog][AlertDialog] Add portal part #936

[Dialog][AlertDialog] Add portal part #936

Uh oh!

Conversation

jjenzz commented Oct 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Notes for docs

Uh oh!

Uh oh!

jjenzz Oct 26, 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!

Uh oh!

jjenzz Oct 26, 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!

jjenzz Oct 26, 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!

jjenzz Oct 26, 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!

jjenzz Oct 26, 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!

jjenzz Oct 26, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benoitgrelard 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!

jjenzz commented Oct 26, 2021 •

edited

Loading

jjenzz Oct 26, 2021 •

edited

Loading

jjenzz Oct 26, 2021 •

edited

Loading

jjenzz Oct 26, 2021 •

edited

Loading

jjenzz Oct 26, 2021 •

edited

Loading

jjenzz Oct 26, 2021 •

edited

Loading

jjenzz Oct 26, 2021 •

edited

Loading