Skip to content
New issue

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

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

Already on GitHub? # to your account

fix(portal): inaccurate hasAttached result and portal being cleared if attached too early #8642

Merged
merged 1 commit into from
Dec 8, 2017

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Nov 25, 2017

Fixes the hasAttached method always returning false when the portal was attached using CdkPortalOutlet.attachComponentPortal or CdkPortalOutlet.attachTemplatePortal. This means that checks like https://github.com/angular/material2/blob/master/src/lib/dialog/dialog-container.ts#L118 always evaluate to false.

Fixing hasAttached revealed another issue: if a portal outlet is unbound (e.g. <div cdkPortalOutlet>) and the consumer attaches something to it on the before the first CD round, the content will be cleared immediately due to Angular invoking the getter with an empty string. This has been fixed by not clearing any previously-attached portals if the reset value is set before ngOnInit.

Fixes #8628.

@crisbeto crisbeto requested a review from jelbourn as a code owner November 25, 2017 10:26
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Nov 25, 2017
@crisbeto crisbeto force-pushed the 8628/portal-has-attached branch from bd21d9c to 0b57725 Compare November 25, 2017 10:30
@crisbeto crisbeto changed the title fix(portal): inaccurate hasAttahed result and portal being cleared if attached too early fix(portal): inaccurate hasAttached result and portal being cleared if attached too early Nov 26, 2017
/** The attached portal. */
private _portal: Portal<any> | null = null;
export class CdkPortalOutlet extends BasePortalOutlet implements OnInit, OnDestroy {
private _hasInitialized = false;
Copy link
Member

Choose a reason for hiding this comment

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

Add description? It's not clear from the name whether this means the directive itself is initialized or something else (maybe _isInitialized?)

… attached too early

Fixes the `hasAttached` method always returning false when the portal was attached using `CdkPortalOutlet.attachComponentPortal` or `CdkPortalOutlet.attachTemplatePortal`. This means that checks like https://github.com/angular/material2/blob/master/src/lib/dialog/dialog-container.ts#L118 always evaluate to false.

Fixing `hasAttached` revealed another issue: if a portal outlet is unbound (e.g. `<div cdkPortalOutlet>`) and the consumer attaches something to it on the before the first CD round, the content will be cleared immediately due to Angular invoking the getter with an empty string. This has been fixed by not clearing any previously-attached portals if the reset value is set before `ngOnInit`.

Fixes angular#8628.
@crisbeto crisbeto force-pushed the 8628/portal-has-attached branch from 0b57725 to cb1b9a0 Compare November 27, 2017 21:51
@crisbeto
Copy link
Member Author

Addressed the feedback @jelbourn.

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Nov 28, 2017
@crisbeto crisbeto added target: minor This PR is targeted for the next minor release target: patch This PR is targeted for the next patch release and removed target: minor This PR is targeted for the next minor release labels Dec 6, 2017
@mmalerba mmalerba merged commit b488b39 into angular:master Dec 8, 2017
mmalerba pushed a commit that referenced this pull request Dec 8, 2017
… attached too early (#8642)

Fixes the `hasAttached` method always returning false when the portal was attached using `CdkPortalOutlet.attachComponentPortal` or `CdkPortalOutlet.attachTemplatePortal`. This means that checks like https://github.com/angular/material2/blob/master/src/lib/dialog/dialog-container.ts#L118 always evaluate to false.

Fixing `hasAttached` revealed another issue: if a portal outlet is unbound (e.g. `<div cdkPortalOutlet>`) and the consumer attaches something to it on the before the first CD round, the content will be cleared immediately due to Angular invoking the getter with an empty string. This has been fixed by not clearing any previously-attached portals if the reset value is set before `ngOnInit`.

Fixes #8628.
@angular-automatic-lock-bot
Copy link

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-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CdkPortalOutlet hasAttached not called when using attachComponentPortal
4 participants