Skip to content

refactor(overlay): use component to render backdrop #6627

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
Closed
wants to merge 1 commit into from
Closed

refactor(overlay): use component to render backdrop #6627

wants to merge 1 commit into from

Conversation

crisbeto
Copy link
Member

Uses an Angular component to render the backdrop, instead of managing a DOM element manually. This has the advantage of being able to leverage the animations API to transition in/out, as well as not having to worry about the cases where the backdrop animation is disabled.

These changes also enable the backdrop transition for the dialog (previously it would be removed immediately on close).

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 24, 2017
jelbourn
jelbourn approved these changes Aug 30, 2017
View reviewed changes
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, just a couple nits

src/cdk/overlay/backdrop.ts Outdated

constructor(private _element: ElementRef, private _renderer: Renderer2) {}

_setClass(cls: string) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd rename cls to either cssClass or just c
(cls has a different meaning for me)

src/cdk/overlay/overlay-ref.ts Outdated
@@ -127,7 +131,8 @@ export class OverlayRef implements PortalHost {
* Returns an observable that emits when the backdrop has been clicked.
*/
backdropClick(): Observable<void> {
return this._backdropClick.asObservable();
return this._backdropInstance ? this._backdropInstance._clickStream.asObservable() :
empty<void>();
Copy link
Member

Choose a reason for hiding this comment

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

Since the function return type is Observable<void>, you don't really need the asObservable call

@crisbeto crisbeto force-pushed the backdrop-component branch from bd6f9c4 to 9a114f5 Compare August 30, 2017 21:18
@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Aug 30, 2017
@crisbeto crisbeto force-pushed the backdrop-component branch 3 times, most recently from 4cb861c to 2dcf30e Compare October 4, 2017 17:27
@kara
Copy link
Contributor

kara commented Oct 4, 2017

@crisbeto Linter strikes again

@kara kara removed the action: merge The PR is ready for merge by the caretaker label Oct 4, 2017
@kara kara assigned crisbeto and unassigned jelbourn Oct 4, 2017
@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Oct 4, 2017
@crisbeto
Copy link
Member Author

crisbeto commented Oct 4, 2017

Fixed.

@kara
Copy link
Contributor

kara commented Oct 5, 2017

@crisbeto CI looks unhappy again

@kara kara removed the action: merge The PR is ready for merge by the caretaker label Oct 5, 2017
@crisbeto crisbeto force-pushed the backdrop-component branch 2 times, most recently from dbdc289 to 0a87d53 Compare October 5, 2017 17:07
@crisbeto
Copy link
Member Author

crisbeto commented Oct 5, 2017 •
edited
Loading

Alright, now the CI should be happy.

@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Oct 5, 2017
@crisbeto crisbeto force-pushed the backdrop-component branch 3 times, most recently from 8214290 to d1eb5db Compare October 11, 2017 16:52
@crisbeto
Copy link
Member Author

crisbeto commented Nov 9, 2017

Rebased again.

@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Nov 9, 2017
@jelbourn jelbourn added the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label Nov 21, 2017
@jelbourn
Copy link
Member

Some Google teams are using detachBackdrop; we'll need to add it back as a deprecated alias.

@crisbeto crisbeto mentioned this pull request Nov 21, 2017
Closed
@crisbeto
Copy link
Member Author

Rebased and re-added the detachOverlay @jelbourn.

@jelbourn jelbourn removed the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label Nov 28, 2017
@crisbeto crisbeto added the target: minor This PR is targeted for the next minor release label Dec 6, 2017
@andrewseguin andrewseguin added the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label Dec 13, 2017
@crisbeto crisbeto mentioned this pull request Jan 25, 2018
Closed
@josephperrott
Copy link
Member

@crisbeto Did you want to rebase and work on getting this one in?

@crisbeto crisbeto force-pushed the backdrop-component branch 3 times, most recently from bc9a464 to a4e7e80 Compare March 31, 2018 09:06
@crisbeto
Copy link
Member Author

Rebased and fixed a few issues @josephperrott.

@crisbeto
refactor(overlay): use component to render backdrop
fe28d4b 
Uses an Angular component to render the backdrop, instead of managing a DOM element manually. This has the advantage of being able to leverage the animations API to transition in/out, as well as not having to worry about the cases where the backdrop animation is disabled.

These changes also enable the backdrop transition for the dialog (previously it would be removed immediately on close).
@crisbeto crisbeto force-pushed the backdrop-component branch from a4e7e80 to fe28d4b Compare April 14, 2018 13:15
@andrewseguin andrewseguin assigned jelbourn and unassigned crisbeto Apr 26, 2018
@jelbourn
Copy link
Member

Going to close this since it's not a big priority and would need further investigation into presubmit failures

@jelbourn jelbourn closed this Apr 26, 2018
@crisbeto crisbeto mentioned this pull request Jun 21, 2018
Closed
@jrood
Copy link
Contributor

jrood commented Jun 21, 2018

Requesting for this issue to be reopened in light of issues highlighted in #11806

@jelbourn
Copy link
Member

We ended up abandoning this because it caused many test failures for various google apps, meaning it would probably cause many more test failures for the community at large.

@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 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn approved these changes

@devversion devversion Awaiting requested review from devversion

@kara kara Awaiting requested review from kara

Assignees

@jelbourn jelbourn

Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged target: minor This PR is targeted for the next minor release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@crisbeto @kara @josephperrott @jelbourn @jrood @googlebot @andrewseguin