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.

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

feat(dialog): add beforeClose method #6377

Merged
merged 4 commits into from
Aug 21, 2017

Conversation

willshowell
Copy link
Contributor

Fixes #4647

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 9, 2017

viewContainerFixture.whenStable().then(() => {
expect(beforeCloseCallback).toHaveBeenCalledWith('Bulbasaurus');
expect(overlayContainerElement.querySelector('md-dialog-container')).toBeNull();
Copy link
Member

Choose a reason for hiding this comment

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

This test checks that beforeClose occurred, but not when it occurred. I'd restructure to be like

let beforeCloseInvoked = false;
const beforeCloseHandler = () => {
  expect(overlayContainerElement.querySelector('md-dialog-container')).toBeDefined();
  beforeCloseInvoked = true;
});
...
viewContainerFixture.whenStable().then(() => {
  expect(beforeCloseCallback).toHaveBeenCalledWith('Bulbasaurus');
  expect(overlayContainerElement.querySelector('md-dialog-container')).toBeNull();
});

@willshowell
Copy link
Contributor Author

@jelbourn update to the test has been made. Let me know if I misinterpreted your comment!

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.

Just one style nit

@@ -140,6 +140,25 @@ describe('MdDialog', () => {
});
}));

it('should close a dialog and get back a result before it is closed', async(() => {
const dialogRef = dialog.open(PizzaMsg, { viewContainerRef: testViewContainerRef });
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 inside braces

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 Aug 17, 2017
@kara kara merged commit cdbf305 into angular:master Aug 21, 2017
@willshowell willshowell deleted the dialog/before-close branch August 21, 2017 23:42
@kdubious
Copy link

I must be missing something, but how do we cancel the close in beforeClose()?

@willshowell
Copy link
Contributor Author

@kdubious check out #4647 (comment)

@kdubious
Copy link

kdubious commented Dec 16, 2017

@willshowell, thanks. I read it, but still have a point of confusion.

I'm trying to pass a DTO to a dialog. The dialog has a form (ReactiveForm) that displays the DTO, the user makes their edits, then clicks "Save." At that point, I want the parent component (it maintains a list of the objects being edited in the dialog) to be notified about the current state of the object, so that the parent can manage saving to a remote service. If unsuccessful, the parent would prevent the dialog from closing.

Is there another approach I should consider?

@willshowell
Copy link
Contributor Author

@kdubious I probably should've linked to this thread #3460, particularly #3460 (comment). Basically beforeClose is simply for getting the close value before the closing animation, but does not allow for synchronous or asynchronous cancellation of the close. If you want to cancel closing of the dialog (via esc or clicking on the backdrop), then you'll want to use disableClose and manually subscribe to the backdrop click and keydown event streams.

It sounds like in your case, you need to run the async validation before the dialog actually closes. If communication with the parent component is the issue, try using a service.

@kdubious
Copy link

Thanks @willshowell. I ended up going with the service.

@wiikka
Copy link

wiikka commented Apr 17, 2018

@willshowell I really don't see any real use case for beforeClose as it is currently implemented? As requested it would make much more sense if you could cancel the close event (e.g. asking from user if he wants to discard the changes).

In that use case it doesn't matter where you get the close event (is it esc, backdrop click or even dialogRef.close()). You should be able to intercept all those and cancel the close if you wish?

@willshowell
Copy link
Contributor Author

@wiikka one of the use cases for beforeClose that was mentioned in the original issue was when you don't care how the dialog was closed, and you don't care to prevent it, but you want to do something with the resulting value without having to wait for the animation to complete.

The conversation in #3460, and more specifically #3460 (comment), pretty much covers why an all-encompassing "prevent/cancel close" api wasn't chosen.

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

Successfully merging this pull request may close these issues.

Add a beforeClose method for MdDialog
6 participants