Skip to content

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Oct 8, 2019

Removes the APIs that were scheduled to be removed from the dialog package in v9.

BREAKING CHANGES:

  • MatDialogRef.afterOpen has been replaced with MatDialogRef.afterOpened.
  • MatDialogRef.beforeClose has been replaced with MatDialogRef.beforeClosed.
  • MatDialog.afterOpen has been replaced with MatDialog.afterOpened.
  • matDialogAnimations.slideDialog has been replaced with matDialogAnimations.dialogContainer.
  • _location parameter in MatDialogRef constructor has been removed.

@crisbeto crisbeto added P2 The issue is important to a large percentage of users, with a workaround target: major This PR is targeted for the next major release labels Oct 8, 2019
@crisbeto crisbeto added this to the 9.0.0 milestone Oct 8, 2019
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 8, 2019
@crisbeto crisbeto force-pushed the dialog-9.0.0-breaking-changes branch from e82b673 to 4236d8b Compare October 8, 2019 21:53
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
Copy link
Member

jelbourn commented Oct 8, 2019

Looks like it just needs API golds updated

@crisbeto crisbeto force-pushed the dialog-9.0.0-breaking-changes branch from 4236d8b to fca880e Compare October 9, 2019 05:47
@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Oct 9, 2019
@jelbourn
Copy link
Member

Breaks nearly 2,000 targets in Google. We'll need to dedicate more time to updating people.

@jelbourn jelbourn added the blocked This issue is blocked by some external factor, such as a prerequisite PR label Oct 11, 2019
@mmalerba mmalerba removed the action: merge The PR is ready for merge by the caretaker label Oct 14, 2019
Removes the APIs that were scheduled to be removed from the dialog package in v9.

BREAKING CHANGES:
* `MatDialogRef.afterOpen` has been replaced with `MatDialogRef.afterOpened`.
* `MatDialogRef.beforeClose` has been replaced with `MatDialogRef.beforeClosed`.
* `MatDialog.afterOpen` has been replaced with `MatDialog.afterOpened`.
* `matDialogAnimations.slideDialog` has been replaced with `matDialogAnimations.dialogContainer`.
* `_location` parameter in `MatDialogRef` constructor has been removed.
@crisbeto crisbeto force-pushed the dialog-9.0.0-breaking-changes branch from fca880e to e0f0571 Compare October 19, 2019 13:16
@mmalerba mmalerba added action: merge The PR is ready for merge by the caretaker and removed blocked This issue is blocked by some external factor, such as a prerequisite PR labels Oct 24, 2019
@mmalerba
Copy link
Contributor

Though there were many broken targets, the underlying cause was only about 20 or so files, working on updating them now

@mmalerba mmalerba merged commit 1d54680 into angular:master Oct 25, 2019
@deviantpixel
Copy link

@crisbeto @jelbourn Can I ask why Location features are being stripped out? I noticed the comment saying they are being deprecated. Is this because of a dependency deprecation or just a decision to deprecate the feature of monitoring the paths to trigger a dialog close?

I ask because I am investigating ways of solving an issue where the dialog remains open during routing changes and ways to allow a feature to "auto-close" the dialog when a routing change occurs. The current "Location" based features is a small part of this and I would be expanding it to use the Router most likely to expand this feature.

Let me know as the information is helpful to know if I should be building this as a solution to be contributed here.

@crisbeto
Copy link
Member Author

These changes remove the _location constructor parameter from MatDialogRef. It hasn't been used for anything for a while now, we just had to wait for a major version to remove it.

@deviantpixel
Copy link

deviantpixel commented Nov 19, 2019

Any interest in an addition that allows for Routing changes to auto-close the Dialog? I would assume it would be an opt-in flag to avoid regression issues. If so I can work on it to contribute it. If this goes against the Dialog assumed features no problem at all.

@crisbeto
Copy link
Member Author

We haven't added closing based on routing, because it would introduce a dependency between Material and the router.

@jelbourn
Copy link
Member

Dialog has a closeOnNavigation option, though, which closes based on the location changing. This used to be implemented in the dialog, but it moved to the lower-level overlay (which is why it's being removed from dialog here)

@deviantpixel
Copy link

deviantpixel commented Nov 20, 2019

Got ya. Makes sense and I did discover that layer. The only issue is I believe that using Location observing which only emits for back/forward browser actions as opposed to clicking links/buttons or other path changes. It's helpful but doesn't cover things like a link in a dialog going to another route in the app or in our case a redirect/auto logout that triggers a route change while the dialog may be open.

Definitely understandable about the Route dependency. I can build it in as a one-off for our implementation. Ping me if there is interest in including this in the future. I will also see if there are ways to do this without needing the Route dependency ;)

@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 Dec 21, 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 P2 The issue is important to a large percentage of users, with a workaround target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants