-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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(snack bar): Add enter and exit animations. #1320
Conversation
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.
For testing, I'm thinking it would make sense to test that the state changes are happening but to not make any assertions about the animations tied to those states.
@@ -16,4 +16,5 @@ $md-snack-bar-max-width: 568px !default; | |||
min-width: $md-snack-bar-min-width; | |||
overflow: hidden; | |||
padding: $md-snack-bar-padding; | |||
transform: translateY(100%); |
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.
Add comment explaining what the initial transform is for.
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.
Done
import {Subject} from 'rxjs/Subject'; | ||
|
||
|
||
export class SnackBarState { |
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 we do this with a string literal type? E.g.,
type SnackBarState = 'initial' | 'visibile' | 'complete' | 'void';
I think this would make the animation definitions much cleaner.
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.
Sure thing, I was thinking that this way allowed us to have a place to hold the values centrally, rather than strings in different places.
state(SnackBarState.visible, style({transform: 'translateY(0%)'})), | ||
state(SnackBarState.complete, style({transform: 'translateY(100%)'})), | ||
transition(`${SnackBarState.visible} => ${SnackBarState.complete}`, | ||
animate('250ms cubic-bezier(0.4, 0.0, 1, 1)')), |
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.
If these are the universal curves for entry and exist, can we drop them into constants in something like core/animation
?
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.
Done, added both duration and easing curves.
}) | ||
export class MdSnackBarContainer extends BasePortalHost { | ||
/** The portal host inside of this container into which the snack bar content will be loaded. */ | ||
@ViewChild(PortalHostDirective) _portalHost: PortalHostDirective; | ||
|
||
/** Subject for notifying that the snack bar has removed from view. */ | ||
private _state: Subject<any> = new Subject(); |
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.
_whenDestroyed
?
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.
Or _onExit
would also work.
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.
Done
private _state: Subject<any> = new Subject(); | ||
|
||
/** The state of the snack bar animations. */ | ||
state: string = SnackBarState.initial; |
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 we call this animationState
?
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.
Done
} | ||
|
||
/** Mark snack bar as exited from the view. */ | ||
markDestroyed(event: AnimationTransitionEvent) { |
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.
markAsDestroyed
?
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.
called markAsExited
let overlayRef = this._createOverlay(); | ||
let snackBarContainer = this._attachSnackBarContainer(overlayRef, config); | ||
let mdSnackBarRef = this._attachSnackbarContent(component, snackBarContainer, overlayRef); | ||
|
||
if (this._snackBarRef) { |
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.
Add comment like
// If there is already a snackbar open, dismiss it and open a new one
// only after the close animation is complete.
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.
Done.
attachTemplatePortal(portal: TemplatePortal): Map<string, any> { | ||
throw Error('Not yet implemented'); | ||
} | ||
|
||
/** Begin animation of the snack bar exiting from view. */ | ||
destroy(): Observable<void> { |
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.
Call this exit
for symmetry with enter
?
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.
Yeah, I initially was looking for this to actually destroy itself, but ultimately just ended up with the exit.
afc2320
to
91a3e3c
Compare
Complex: '375ms', | ||
Entering: '225ms', | ||
Exiting: '195ms', | ||
}; |
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 you change these constants to the static class format (how you had the enum for animation state earlier), e.g.,
export class AnimationDuration {
static Complex = '375ms';
...
}
Also, I think we can combine this file and the next into just animation.ts
(it's fine that it's animation/animation.ts
)
@@ -16,4 +16,6 @@ $md-snack-bar-max-width: 568px !default; | |||
min-width: $md-snack-bar-min-width; | |||
overflow: hidden; | |||
padding: $md-snack-bar-padding; | |||
/** Initial transformation is applied to start snack bar out of view. */ |
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 you use the //
style comments? I recently changed all of the scss comments to //
when I did the theming.
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.
Done
/** The snack bar configuration. */ | ||
snackBarConfig: MdSnackBarConfig; | ||
|
||
/** Attach a portal as content to this snack bar container. */ | ||
constructor(private ngZone: NgZone) { |
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.
_ngZone
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.
Done
markAsExited(event: AnimationTransitionEvent) { | ||
if (event.fromState === 'visible' && | ||
(event.toState === 'void' || event.toState === 'complete')) { | ||
this.ngZone.run(() => this._state.complete()); |
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.
You should call next()
in addition to complete()
, that was there's still an "event" in the stream and not just the completion of the stream. This lets you subscribe like
container.exit().subscribe(() => { ... });
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.
Done
}) | ||
export class MdSnackBarContainer extends BasePortalHost { | ||
/** The portal host inside of this container into which the snack bar content will be loaded. */ | ||
@ViewChild(PortalHostDirective) _portalHost: PortalHostDirective; | ||
|
||
/** Subject for notifying that the snack bar has removed from view. */ | ||
private _state: Subject<any> = new Subject(); |
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.
Or _onExit
would also work.
this._afterClosed.complete(); | ||
this.containerInstance.exit().subscribe(null, null, () => { | ||
this._overlayRef.dispose(); | ||
this._afterClosed.complete(); |
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.
next()
in addition to complete()
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.
Done
|
||
this._snackBarRef = snackBarRef; | ||
return snackBarRef; | ||
return <MdSnackBarRef<T>> new MdSnackBarRef(contentRef.instance, |
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.
I want to say that you shouldn't need the typecast, but I'm too jet lagged to think any deeper about 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.
You are correct, not sure what I was doing before that I needed to cast it.
a27b2fc
to
7ec6fd3
Compare
7ec6fd3
to
69a0ccf
Compare
4aef5ea
to
42febfb
Compare
fa83858
to
fbb2d4f
Compare
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 aside from one last nit
static get standardCurve(): string { return 'cubic-bezier(0.4,0.0,0.2,1)'; } | ||
static get decelerationCurve(): string { return 'cubic-bezier(0.0,0.0,0.2,1)'; } | ||
static get accelerationCurve(): string { return 'cubic-bezier(0.4,0.0,1,1)'; } | ||
static get sharpCurve(): string { return 'cubic-bezier(0.4,0.0,0.6,1)'; } |
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.
No need for these to be getters; you can just have
static STANDARD_CURVE = 'cubic-bezier(0.4,0.0,0.2,1)';
419a01e
to
a654207
Compare
a654207
to
ccc2bdc
Compare
Hey Joey, I don't see that these tests are evaluating the "afterDismissed" subscription. Anything being expected inside these parts of the tests are not being called because the animation done state is not triggering. Were you able to get these working in the tests? Example of no-op: |
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. |
Continuing #115