-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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): support open with TemplateRef #2910
Conversation
|
||
/** | ||
* Moves the focus inside the focus trap. | ||
* @private |
There was a problem hiding this comment.
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
Can you add a test for using template. LGTM otherwise, add merge ready when you are good with it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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? |
@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 |
@mmalerba the main difference is that I created a new method |
@fxck Ok, lets merge this one and then do another PR to add the observables you want. |
It would have been really nice to include an update to the docs for this feature. |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
No description provided.