Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow nested sheets without boilerplate #5660

Merged
merged 5 commits into from
Oct 9, 2024
Merged

Allow nested sheets without boilerplate #5660

merged 5 commits into from
Oct 9, 2024

Conversation

mozzius
Copy link
Member

@mozzius mozzius commented Oct 9, 2024

Due to the strict necessity of iOS native sheets needing to be nested, we were creating new portal groups manually and then prop drilling to the sub-dialogs. This felt like a leaky abstraction, so this PR handles this automatically ✨

This has let me delete all of the unnecessary prop drilling in more cases. I've kept the prop around for the more general-purpose components for if we want to use it in future, but they're no longer used

How it works

  1. Dialogs get the Portal they use from context. the default value of the context is same as before (thus no top-level provider needed) (also can be overridden by prop still)
  2. Dialogs set a new default portal provider for their children via <DefaultPortalOverride>

Test plan

  1. Test normal dialogs work as before
  2. Test nested dialogs work: within composer, prompt in muted words, "dialog with menu in it" in the storybook
  3. Make sure it roughly works the same on web - I opted not to override the default portal for dialog content there

@arcalinea arcalinea temporarily deployed to samuel/auto-portal - social-app PR #5660 October 9, 2024 14:11 — with Render Destroyed
Copy link

github-actions bot commented Oct 9, 2024

Old size New size Diff
7.89 MB 7.89 MB 3.6 KB (0.04%)

@arcalinea arcalinea temporarily deployed to samuel/auto-portal - social-app PR #5660 October 9, 2024 14:23 — with Render Destroyed
@@ -50,8 +50,10 @@ export function Outer({
onClose,
nativeOptions,
testID,
Portal = DefaultPortal,
Portal: PortalProp,
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hailey seemed to want to keep it around, I'd be fine to rm personally

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yea this is fine to RM now 👍

@@ -72,3 +72,23 @@ const DefaultPortal = createPortalGroup()
export const Provider = DefaultPortal.Provider
export const Outlet = DefaultPortal.Outlet
export const Portal = DefaultPortal.Portal

// when in native sheets, we want the default Portal to move to inside the sheet for children of the sheet
export const DefaultPortalContext = React.createContext(DefaultPortal.Portal)
Copy link
Member

Choose a reason for hiding this comment

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

Is "default" the right word here? I feel like we could just call it PortalContext. The default is DefaultPortal, which is aptly named in this file. But this context overrides, and in the spirit of "not thinking about it", I think usePortal or like useParentPortal might make more sense? No strong feelings, just spitballing.

Copy link
Member Author

Choose a reason for hiding this comment

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

useDefaultPortal makes sense in the context of it maybe being passed in via prop instead. you shouldn't need to know that the default portal might get overridden if in a sheet

Copy link
Member

@estrattonbailey estrattonbailey left a comment

Choose a reason for hiding this comment

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

LGTM

@arcalinea arcalinea temporarily deployed to samuel/auto-portal - social-app PR #5660 October 9, 2024 18:29 — with Render Destroyed
Copy link
Contributor

@haileyok haileyok left a comment

Choose a reason for hiding this comment

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

Thank you for all the feedback on this 🙏

@haileyok haileyok merged commit cca344a into main Oct 9, 2024
6 checks passed
@haileyok haileyok deleted the samuel/auto-portal branch October 9, 2024 18:30
haileyok pushed a commit that referenced this pull request Oct 10, 2024
Co-authored-by: Hailey <me@haileyok.com>
(cherry picked from commit cca344a)
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.

4 participants