Skip to content

feat(portal): add new portal that projects DOM nodes - #16101

Merged
mmalerba merged 1 commit into
angular:masterfrom
crisbeto:element-portal
Nov 7, 2019
Merged

feat(portal): add new portal that projects DOM nodes#16101
mmalerba merged 1 commit into
angular:masterfrom
crisbeto:element-portal

Conversation

@crisbeto

@crisbetocrisbeto commented May 23, 2019

Copy link
Copy Markdown
Member

Adds a new type of portal called DomPortal which transfers the contents of a portal into the portal outlet and then restores them on destroy.

This was implemented initially for #14430.

@googlebotgooglebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 23, 2019
@crisbetocrisbeto added the target: minor This PR is targeted for the next minor release label May 23, 2019
@crisbeto
crisbeto marked this pull request as ready for review May 23, 2019 19:42
jelbourn
jelbourn reviewed May 24, 2019
View reviewed changes
Comment threadsrc/cdk/portal/portal.ts Outdated
Comment threadsrc/cdk/portal/portal.ts
Comment threadsrc/cdk/portal/portal.ts Outdated
Comment threadsrc/cdk/portal/dom-portal-outlet.ts Outdated
@crisbeto

crisbeto commented May 27, 2019

Copy link
Copy Markdown
MemberAuthor

I've reworked it based on the feedback @jelbourn.

wnvko
wnvko reviewed May 27, 2019
View reviewed changes
Comment threadsrc/cdk/portal/dom-portal-outlet.ts Outdated
@crisbeto

crisbeto commented Jun 24, 2019

Copy link
Copy Markdown
MemberAuthor

I've reworked it to move the entire portal element, rather than the content. Can you take another look @jelbourn@wnvko?

@wnvko

wnvko commented Jun 25, 2019

Copy link
Copy Markdown
Contributor

LGTM

jelbourn
jelbourn reviewed Jun 25, 2019
View reviewed changes
Comment threadsrc/cdk/portal/dom-portal-outlet.ts
Comment threadsrc/cdk/portal/portal.ts Outdated
@crisbeto

crisbeto commented Jun 25, 2019

Copy link
Copy Markdown
MemberAuthor

The latest set of feedback has been addressed @jelbourn.

jelbourn
jelbourn approved these changes Sep 5, 2019
View reviewed changes

@jelbournjelbourn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, just needs rebase

Comment threadsrc/cdk/portal/dom-portal-outlet.ts Outdated
@crisbeto
crisbetoforce-pushed the element-portal branch 2 times, most recently from 463ed62 to d3ad339CompareSeptember 5, 2019 19:56
@crisbeto

crisbeto commented Sep 5, 2019

Copy link
Copy Markdown
MemberAuthor

Rebased.

@crisbetocrisbeto added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent action: merge The PR is ready for merge by the caretaker labels Sep 5, 2019
@crisbeto
crisbeto requested a review from a teamSeptember 5, 2019 20:05
@jelbourn

jelbourn commented Sep 6, 2019

Copy link
Copy Markdown
Contributor

@crisbeto both Google Analytics and Google Cloud extend BasePortalOutlet with custom outlets, so they break due to missing attachDomPortal. What do you think of adding a default implementation to the abstract class that throws a "not implemented" error? We can then remove that default impl in a major version

@crisbetocrisbeto removed the target: minor This PR is targeted for the next minor release label Sep 10, 2019
@crisbetocrisbeto added the target: major This PR is targeted for the next major release label Sep 10, 2019
@crisbetocrisbeto added this to the 9.0.0 milestone Nov 2, 2019
mmalerba
mmalerba reviewed Nov 5, 2019
View reviewed changes
Comment threadsrc/cdk/portal/portal.ts Outdated
@crisbeto
feat(portal): add new portal that projects DOM nodes
94652fd
Adds a new type of portal called `DomPortal` which transfers the contents of a portal into the portal outlet and then restores them on destroy.
This was implemented initially for angular#14430.
@crisbeto

crisbeto commented Nov 6, 2019

Copy link
Copy Markdown
MemberAuthor

Updated @mmalerba.

@mmalerba
mmalerba merged commit d3d8859 into angular:masterNov 7, 2019
@angular-automatic-lock-bot

angular-automatic-lock-botBot commented Dec 8, 2019

Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-botangular-automatic-lock-botBot locked and limited conversation to collaborators Dec 8, 2019
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@josephperrottjosephperrottAwaiting requested review from josephperrott
@andrewseguinandrewseguinAwaiting requested review from andrewseguin
3 more reviewers
@mmalerbammalerbammalerba left review comments
@jelbournjelbournjelbourn approved these changes
@wnvkownvkownvko approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@jelbournjelbourn

Labels

action: mergeThe PR is ready for merge by the caretakercla: yesPR author has agreed to Google's Contributor License AgreementP3An issue that is relevant to core functions, but does not impede progress. Important, but not urgenttarget: majorThis PR is targeted for the next major release

Projects

None yet

Milestone

9.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@crisbeto@wnvko@jelbourn@mmalerba@googlebot