Skip to content

Conversation

mmalerba
Copy link
Contributor

@mmalerba mmalerba commented Feb 3, 2017

No description provided.

@mmalerba mmalerba requested review from kara and andrewseguin February 3, 2017 00:07
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Feb 3, 2017

/**
* Moves the focus inside the focus trap.
* @private
Copy link
Contributor

Choose a reason for hiding this comment

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

Can remove the @private part of the jsdoc since its in the function signature

@andrewseguin
Copy link
Contributor

Can you add a test for using template. LGTM otherwise, add merge ready when you are good with it

@mmalerba mmalerba added the action: merge The PR is ready for merge by the caretaker label Feb 3, 2017
Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

LGTM

@fxck
Copy link
Contributor

fxck commented Feb 4, 2017

Is this replacement for #2853 then? Should I make a PR with stuff discussed here #2853 (comment) after this PR and this PR gets merged(it needs rebase again btw @crisbeto)? How does merging workflow in this repository work anyway? Do you merge PRs labelled merge ready once per two weeks?

@mmalerba mmalerba removed the action: merge The PR is ready for merge by the caretaker label Feb 5, 2017
@mmalerba
Copy link
Contributor Author

mmalerba commented Feb 5, 2017

@fxck Ah my bad, I didn't realize there was already a PR in progress for this. Let me take a look at yours. I'm hoping to get this feature in soon so I can use it for datepicker. Removed merge-ready until I have a chance to compare

@fxck
Copy link
Contributor

fxck commented Feb 5, 2017

@mmalerba the main difference is that I created a new method openFromTemplate, since I didn't know you could "cheat" TemplatePortal by giving it null as ViewContainerRef. I also exposed backdropClick and escapePress events for the reasons explained in the comment I linked previously. I didn't really finish the PR since I wanted to base it on @crisbeto's PR with enter/exit animation which changes quite a few things in overlay/backdrop. All in all, I'm fine with your PR replacing mine, I just really want those backdrop/escape events exposed so I can do this #2855

@mmalerba
Copy link
Contributor Author

mmalerba commented Feb 6, 2017

@fxck Ok, lets merge this one and then do another PR to add the observables you want.

@mmalerba mmalerba added the action: merge The PR is ready for merge by the caretaker label Feb 6, 2017
@tinayuangao tinayuangao merged commit bf0f625 into angular:master Feb 9, 2017
@Splaktar
Copy link
Contributor

Splaktar commented May 4, 2017

It would have been really nice to include an update to the docs for this feature.

@mmalerba mmalerba deleted the dlg branch April 3, 2018 15:16
@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.

7 participants