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.

Sign up for GitHub

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

Jump to bottom

[popups] Create Portal part #937

Merged
merged 41 commits into from
Dec 11, 2024
Merged

[popups] Create Portal part #937

merged 41 commits into from
Dec 11, 2024

Conversation

atomiks
Copy link
Contributor

@atomiks atomiks commented Dec 2, 2024

@atomiks atomiks added core Infrastructure work going on behind the scenes component: select This is the name of the generic UI component, not the React module! component: menu This is the name of the generic UI component, not the React module! component: alert dialog This is the name of the generic UI component, not the React module! component: tooltip This is the name of the generic UI component, not the React module! component: dialog This is the name of the generic UI component, not the React module! component: popover The React component. component: preview card The React component. labels Dec 2, 2024
@mui-bot
Copy link

mui-bot commented Dec 2, 2024 •
edited
Loading

Netlify deploy preview

https://deploy-preview-937--base-ui.netlify.app/

Generated by 🚫 dangerJS against 14d7c3b

@atomiks atomiks marked this pull request as ready for review December 3, 2024 13:21
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 3, 2024
@colmtuite colmtuite requested a review from vladmoroz December 4, 2024 10:48
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 4, 2024
@michaldudak
Copy link
Member

It seems to me we could gain quite a lot by inverting the dependencies. Instead of a separate *Portal component, we could have one (internal) Portal (re-exported as a part of each component) with its own PortalContext. Then, each Root part would set up this context.
The benefits I can think of are a smaller package, one place to introduce changes and fewer unnecessary rerenders (Portal would rerender only if its context changes, not the large *RootContext).
Backdrops could also benefit from this, and ideally Positioners, but this could be more tricky as they differ between components.

michaldudak
michaldudak reviewed Dec 5, 2024
View reviewed changes
packages/react/src/alert-dialog/portal/AlertDialogPortal.tsx Outdated
) {
const { children, container, keepMounted = false } = props;

const { mounted } = useAlertDialogRootContext();
Copy link
Member

Choose a reason for hiding this comment

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

We agreed to use the term visible instead of mounted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mounted is used everywhere currently and doesn't relate to this PR. We can change it but it's not necessary/not exposed to publicly right now

@atomiks
Copy link
Contributor Author

atomiks commented Dec 6, 2024

It seems to me we could gain quite a lot by inverting the dependencies. Instead of a separate *Portal component, we could have one (internal) Portal (re-exported as a part of each component) with its own PortalContext. Then, each Root part would set up this context.

Not sure I fully understand - these separate Portal components are really small, I think the context would add more size if anything. Portal re-rendering is a non-issue since it's cheap

@michaldudak
Copy link
Member

They are small, but they are copied 7 times. Defining it just once (plus the context, which will be even smaller, I suppose) would make the package lighter.

I realize that rendering a Portal isn't a perf bottleneck, but if we can easily save a few CPU cycles, why not?

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 6, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 9, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 9, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 10, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 10, 2024
atomiks
atomiks commented Dec 11, 2024
View reviewed changes
scripts/buildApiDocs/config/getBaseUiComponentInfo.ts
Comment on lines -30 to +35
const customAnnotation = `Documentation: [Base UI ${parentComponentName}](${url})`;
// Foreign components that aren't associated with any given component.
const EXCLUDED_NAMES = ['Portal'];

const customAnnotation = EXCLUDED_NAMES.includes(name)
? `Documentation: https://base-ui.com`
: `Documentation: [Base UI ${parentComponentName}](${url})`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Foreign standalone components (which aren't used standalone in terms of the public API and have no dedicated page) can't be associated to their component without wrapping. We can either always wrap (which is already required for Select) or just link to the homepage?

michaldudak
michaldudak approved these changes Dec 11, 2024
View reviewed changes
packages/react/src/portal/PortalContext.ts
@@ -0,0 +1,13 @@
import * as React from 'react';

export const PortalContext = React.createContext<boolean | undefined>(undefined);
Copy link
Member

Choose a reason for hiding this comment

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

Add the displayName

@atomiks atomiks merged commit 5864f86 into mui:master Dec 11, 2024
23 checks passed
@atomiks atomiks deleted the feat/portal branch December 11, 2024 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaldudak michaldudak michaldudak approved these changes

@colmtuite colmtuite Awaiting requested review from colmtuite colmtuite is a code owner

@vladmoroz vladmoroz Awaiting requested review from vladmoroz

Assignees

@atomiks atomiks

Labels
component: alert dialog This is the name of the generic UI component, not the React module! component: dialog This is the name of the generic UI component, not the React module! component: menu This is the name of the generic UI component, not the React module! component: popover The React component. component: preview card The React component. component: select This is the name of the generic UI component, not the React module! component: tooltip This is the name of the generic UI component, not the React module! core Infrastructure work going on behind the scenes
Projects
Base UI Alpha
Status: Done
Milestone
1.0.0-alpha.1 release
Development

Successfully merging this pull request may close these issues.
4 participants
@atomiks @mui-bot @michaldudak @colmtuite