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

feat(dialog): add events (observables) for open & closeAll #2522

Merged
merged 10 commits into from
Jan 26, 2017
Merged

feat(dialog): add events (observables) for open & closeAll #2522

merged 10 commits into from
Jan 26, 2017

Conversation

thomaspink
Copy link
Contributor

Adds Observables to NgDialog for open and closeAll.

  • afterOpen: Notifies the user that a dialog has been opened
  • afterAllClosed: Notifies the user that all open dialogs have finished closing.

Useful if you want to execute logic when a dialog opens or when all dialogs are closed. E.g. adding or removing a no-scroll class to the body, ...

  dialog.afterOpen().subscribe((ref: MdDialogRef<any>) => {
    if (!doc.body.classList.contains('no-scroll')) {
      doc.body.classList.add('no-scroll');
    }
  });
  dialog.afterAllClosed().subscribe(_ => {
    doc.body.classList.remove('no-scroll');
  });

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 4, 2017
@thomaspink
Copy link
Contributor Author

thomaspink commented Jan 4, 2017 •
edited
Loading

Browserstack build failing on iPhone 6s. Don't get it why ... does someone have a hint/tip for me?
https://travis-ci.org/angular/material2/jobs/188875161

EDIT: Now green

@devversion
Copy link
Member

@thomaspink I restarted your build now, and it's now green.

@thomaspink
Copy link
Contributor Author

@devversion thank you.

what was the problem?

@devversion
Copy link
Member

devversion commented Jan 4, 2017 •
edited
Loading

@thomaspink This sometimes happens when too many builds run at the same time. #2523

@thomaspink
Copy link
Contributor Author

Good to know, thanks :)

jelbourn
jelbourn reviewed Jan 13, 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.

@thomaspink sorry for the review delay, we're a bit backlogged after the holidays.

Overall I think this is a useful feature. Could you rebase on current master? We've made some changes recently to the way that MdDialog maintains state; if there is an ancestor MdDialog instance, it defers its state to that ancestor.

src/lib/dialog/dialog.ts
@@ -59,6 +68,20 @@ export class MdDialog {
}

/**
* Gets an observable that is notified when a dialog has been opened.
*/
afterOpen(): Observable<any> {
Copy link
Member

Choose a reason for hiding this comment

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

This is okay to just be a property rather than a method (in hindsight we shouldn't have made the afterClosed on the ref a method and will be changing it).

src/lib/dialog/dialog.ts
private _afterAllClosed: Subject<any> = new Subject();

/** Subject for notifying the user that a dialog has opened. */
private _afterOpen: Subject<any> = new Subject();
Copy link
Member

Choose a reason for hiding this comment

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

This would be Subject<MdDialogRef<any>>

src/lib/dialog/dialog.ts
@@ -22,6 +24,12 @@ export class MdDialog {
/** Keeps track of the currently-open dialogs. */
private _openDialogs: MdDialogRef<any>[] = [];

/** Subject for notifying the user that all open dialogs have finished closing. */
private _afterAllClosed: Subject<any> = new Subject();
Copy link
Member

Choose a reason for hiding this comment

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

This one would be Subject<void>

@jelbourn jelbourn self-assigned this Jan 13, 2017
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

@googlebot googlebot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Jan 13, 2017
@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Jan 13, 2017
@thomaspink
Copy link
Contributor Author

thomaspink commented Jan 13, 2017 •
edited
Loading

@jelbourn thanks for the feedback.
Please review the latest changes.

Should i also change the afterClose function to a property in this/new PR or are you guys doing that?

I am also not that happy with the name afterOpen. Maybe change it to onOpen?

jelbourn
jelbourn reviewed Jan 14, 2017
View reviewed changes
src/lib/dialog/dialog.ts
private _afterAllClosed = new Subject<void>();

/** Subject for notifying the user that a dialog has opened. */
private _afterOpen = new Subject<MdDialogRef<any>>();
Copy link
Member

Choose a reason for hiding this comment

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

The _afterAllClosed and _afterOpen subjects need to delegate to the state of the _parentDialog if there is one (the same way that _openDialogs does. This is because the application may contain multiple MdDialog instances at different points in the tree.

src/demo-app/dialog/dialog-demo.ts
@@ -1,4 +1,5 @@
import {Component} from '@angular/core';
import { Component, Inject } from '@angular/core';
import { DOCUMENT } from '@angular/platform-browser';
Copy link
Member

Choose a reason for hiding this comment

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

Omit spaces from inside import braces

@thomaspink
Copy link
Contributor Author

thomaspink commented Jan 14, 2017 •
edited
Loading

@jelbourn implemented as suggested :)

jelbourn
jelbourn approved these changes Jan 24, 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

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Jan 24, 2017
@andrewseguin andrewseguin merged commit 23ab152 into angular:master Jan 26, 2017
@thomaspink thomaspink deleted the dialog-close-all-observable branch January 27, 2017 14:53
@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 6, 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

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
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@thomaspink @devversion @googlebot @jelbourn @andrewseguin