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(datepicker): remove dependency on mat-dialog #13019

Closed
wants to merge 1 commit into from

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Sep 7, 2018

Reworks the datepicker to remove its dependency to material/dialog, avoiding bringing in all of the overhead of the dialog from which the datepicker is using a small fraction.

@crisbeto crisbeto added the target: minor This PR is targeted for the next minor release label Sep 7, 2018
@crisbeto crisbeto requested a review from mmalerba as a code owner September 7, 2018 08:24
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 7, 2018
@ngbot
Copy link

ngbot bot commented Sep 18, 2018

Hi @crisbeto! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

1 similar comment
@ngbot
Copy link

ngbot bot commented Sep 18, 2018

Hi @crisbeto! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

@crisbeto crisbeto force-pushed the datepicker-dialog-decouple branch from b53813c to 8c915e9 Compare September 19, 2018 20:49
@mmalerba mmalerba added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Oct 2, 2018
Copy link
Contributor

@mmalerba mmalerba left a comment

Choose a reason for hiding this comment

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

Looks good, we'll have to see how the presubmit goes though. I don't know whether people are relying on this being an actual dialog in tests


if (!this.touchUi) {
// Update the position once the calendar has rendered.
this._ngZone.onStable.asObservable()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can leave off the .asObservable()

Copy link
Member Author

Choose a reason for hiding this comment

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

I've been doing this since, at some point in the past, omitting it ended up causing a memory leak. I'm not sure whether that hasn't been fixed in core by now though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok, didn't realize that

@crisbeto crisbeto force-pushed the datepicker-dialog-decouple branch from 8c915e9 to 3164d94 Compare October 27, 2018 16:00
@crisbeto crisbeto force-pushed the datepicker-dialog-decouple branch from 3164d94 to 43a29b5 Compare December 13, 2018 17:43
@crisbeto crisbeto force-pushed the datepicker-dialog-decouple branch from 43a29b5 to 79fdefe Compare December 29, 2018 16:14
@mmalerba mmalerba added aaa and removed aaa labels Apr 25, 2019
@vlapo
Copy link

vlapo commented Jul 6, 2019

What is status of this? Do you plan to merge this? We would like to use touchUi but we have a few issues related to dialog and #7390. (mostly on ios safari)

Reworks the datepicker to remove its dependency to `material/dialog`, avoiding bringing in all of the overhead of the dialog from which the datepicker is using a small fraction.
@crisbeto crisbeto force-pushed the datepicker-dialog-decouple branch from 79fdefe to abd743a Compare July 19, 2019 19:44
@mmalerba mmalerba removed their assignment Aug 12, 2019
@mmalerba mmalerba removed the lgtm label Jul 31, 2020
crisbeto added a commit to crisbeto/material2 that referenced this pull request Apr 1, 2021
Currently the datepicker has a hard dependency on `MatDialog` due to its touch-specific appearance which makes it difficult to use together with the MDC packages, because they have an alternate dialog module.

These changes resolve the issue by removing the datepicker's dependency on `MatDialog`.  The approach is loosely based on angular#13019, however I've simplified things a bit to make it more maintainable.
@crisbeto
Copy link
Member Author

crisbeto commented Apr 1, 2021

Closing in favor of #22383.

@crisbeto crisbeto closed this Apr 1, 2021
crisbeto added a commit to crisbeto/material2 that referenced this pull request Apr 8, 2021
Currently the datepicker has a hard dependency on `MatDialog` due to its touch-specific appearance which makes it difficult to use together with the MDC packages, because they have an alternate dialog module.

These changes resolve the issue by removing the datepicker's dependency on `MatDialog`.  The approach is loosely based on angular#13019, however I've simplified things a bit to make it more maintainable.
crisbeto added a commit to crisbeto/material2 that referenced this pull request Apr 8, 2021
Currently the datepicker has a hard dependency on `MatDialog` due to its touch-specific appearance which makes it difficult to use together with the MDC packages, because they have an alternate dialog module.

These changes resolve the issue by removing the datepicker's dependency on `MatDialog`.  The approach is loosely based on angular#13019, however I've simplified things a bit to make it more maintainable.
mmalerba pushed a commit that referenced this pull request Apr 13, 2021
Currently the datepicker has a hard dependency on `MatDialog` due to its touch-specific appearance which makes it difficult to use together with the MDC packages, because they have an alternate dialog module.

These changes resolve the issue by removing the datepicker's dependency on `MatDialog`.  The approach is loosely based on #13019, however I've simplified things a bit to make it more maintainable.
@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 May 2, 2021
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 target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants