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): add animation to calendar popup #8542

Merged
merged 1 commit into from
Dec 13, 2017

Conversation

crisbeto
Copy link
Member

Adds an animation when opening and closing the datepicker's calendar.

For reference:
animation

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Nov 19, 2017
@crisbeto crisbeto force-pushed the datepicker-calendar-animation branch from 8493b5d to 83c7e2c Compare November 19, 2017 13:58
@ghost
Copy link

ghost commented Nov 19, 2017

It would be very cool if the sheet would do also a opening along the x-axis like here.

@crisbeto
Copy link
Member Author

@theredd0t that's what I was going for initially (the menu has a similar animation), but it looks weird when the animation doesn't originate from the place where the user clicked.

@ghost
Copy link

ghost commented Nov 19, 2017

@crisbeto Yeah I just realized that this would be a "Don't" according to the guidelines.

https://material.io/guidelines/motion/choreography.html#choreography-creation

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 fantastic, thanks!

ngAfterContentInit() {
this._calendar._focusActiveCell();
}

ngOnDestroy() {
if (this._positionChange) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should we use the usual _destoryed.next()?

Copy link
Member Author

@crisbeto crisbeto Nov 20, 2017

Choose a reason for hiding this comment

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

I think for this case it's better to go with the subscription, because it allows us to know whether we've subscribed before. Otherwise since we don't dispose of the datepicker content until the datepicker is destroyed, we could end up in a situation where we've subscribed multiple times to the same stream.

@mmalerba
Copy link
Contributor

Just realized, since this adds an Angular animation to a component that previously didn't have one, it may be a breaking change.

@jelbourn should this be part of a patch, minor, or major?

Adds an animation when opening and closing the datepicker's calendar.
@crisbeto crisbeto force-pushed the datepicker-calendar-animation branch from 83c7e2c to 3dca420 Compare November 20, 2017 20:09
@jelbourn
Copy link
Member

I chatted w/ @StephenFluin and he agreed that adding an animation to a component that didn't have one before would be considered a breaking change since users would have to add BrowserAnimationsModule if they didn't have it (however unlikely it is that someone is using datepicker and not some other component that has animations already). I think this one may have to wait for 6.0

@jelbourn jelbourn added the target: major This PR is targeted for the next major release label Nov 22, 2017
@mmalerba mmalerba added this to the 6.0 milestone Nov 24, 2017
@mmalerba
Copy link
Contributor

@jelbourn for 6.0 we might want to consider adding a no-op animation to anything that's supposed to have animations but we haven't got to it yet. That way we can do it at some point without it being breaking

@jelbourn
Copy link
Member

Unless something changes in Angular, we won't be able to do that; both animations modules import BrowserModule, which Angular only allows to be imported once.

@mmalerba
Copy link
Contributor

@jelbourn By no-op animation I don't mean the NoopAnimationModule, just an animation that does nothing, e.g. state('noop', style({})). This way it would have the BrowserAnimationsModule dependency and we could later make the animation do useful things without it being a breaking change

@mmalerba mmalerba added the action: merge The PR is ready for merge by the caretaker label Dec 6, 2017
@andrewseguin andrewseguin merged commit c3e267f into angular:master Dec 13, 2017
crisbeto added a commit to crisbeto/material2 that referenced this pull request Dec 14, 2017
Adds an animation when opening and closing the datepicker's calendar.

This is a resubmit of angular#8542.
crisbeto added a commit to crisbeto/material2 that referenced this pull request Feb 9, 2018
Adds an animation when opening and closing the datepicker's calendar.

This is a resubmit of angular#8542.
crisbeto added a commit to crisbeto/material2 that referenced this pull request Feb 13, 2018
Adds an animation when opening and closing the datepicker's calendar.

This is a resubmit of angular#8542.
crisbeto added a commit to crisbeto/material2 that referenced this pull request Feb 19, 2018
Adds an animation when opening and closing the datepicker's calendar.

This is a resubmit of angular#8542.
mmalerba pushed a commit to crisbeto/material2 that referenced this pull request Mar 8, 2018
Adds an animation when opening and closing the datepicker's calendar.

This is a resubmit of angular#8542.
mmalerba pushed a commit that referenced this pull request Mar 8, 2018
Adds an animation when opening and closing the datepicker's calendar.

This is a resubmit of #8542.
crisbeto added a commit to crisbeto/material2 that referenced this pull request Mar 8, 2018
Adds an animation when opening and closing the datepicker's calendar.

This is a resubmit of angular#8542.
@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 7, 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 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