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

[Portal] Kill old portal and replace with new one #1460

Merged
merged 2 commits into from
Jun 10, 2022
Merged

[Portal] Kill old portal and replace with new one #1460

merged 2 commits into from
Jun 10, 2022

Conversation

benoitgrelard
Copy link
Contributor

@benoitgrelard benoitgrelard commented Jun 9, 2022 •
edited
Loading

Note: needs to be rebased once #1459 is merged

Warning
Breaking changes in docs:

  • Breaking changes for Portal util: manual z-index management + containerRef to container prop
  • update docs page for Portal

@benoitgrelard benoitgrelard added PR: Breaking Change This PR contains a breaking change PR: Documentation Needed This PR warrants a documentation update labels Jun 9, 2022
@benoitgrelard benoitgrelard requested a review from andy-hook as a code owner June 9, 2022 16:38
jjenzz
jjenzz approved these changes Jun 9, 2022
View reviewed changes
Copy link
Contributor

@jjenzz jjenzz left a comment

Choose a reason for hiding this comment

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

;)

andy-hook
andy-hook reviewed Jun 10, 2022
View reviewed changes
packages/react/toast/src/Toast.tsx
@@ -4,7 +4,7 @@ import { composeEventHandlers } from '@radix-ui/primitive';
import { useComposedRefs } from '@radix-ui/react-compose-refs';
import { createContextScope } from '@radix-ui/react-context';
import * as DismissableLayer from '@radix-ui/react-dismissable-layer';
import { UnstablePortal } from '@radix-ui/react-portal';
import { Portal } from '@radix-ui/react-portal';
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we alias in some places but not others?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because a lot of the new components now expose their own Portal part so it would clash.

Copy link
Contributor

Choose a reason for hiding this comment

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

Was just about to update my comment, should we be consistent? does it matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it matter much, and generally I think we only alias when the need is there.

@benoitgrelard benoitgrelard merged commit 8379544 into select-parity Jun 10, 2022
@benoitgrelard benoitgrelard deleted the kill-old-portal branch June 10, 2022 10:47
@benoitgrelard
Copy link
Contributor Author

I just realised I fucked this up here, because it was merged into select-parity branch and then I rebased it, I thought it had gone into main.

The order of merging should have been the other way around…

@benoitgrelard benoitgrelard restored the kill-old-portal branch June 10, 2022 12:26
@benoitgrelard benoitgrelard mentioned this pull request Jun 10, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andy-hook andy-hook andy-hook approved these changes

@jjenzz jjenzz jjenzz approved these changes

Assignees
No one assigned
Labels
PR: Breaking Change This PR contains a breaking change PR: Documentation Needed This PR warrants a documentation update
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@benoitgrelard @jjenzz @andy-hook