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(snack bar): Add enter and exit animations. #1320

Merged
merged 8 commits into from
Oct 18, 2016

Conversation

josephperrott
Copy link
Member

Continuing #115

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 24, 2016
Copy link
Member

@jelbourn jelbourn left a 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%);
Copy link
Member

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.

Copy link
Member Author

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 {
Copy link
Member

@jelbourn jelbourn Sep 25, 2016

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.

Copy link
Member Author

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)')),
Copy link
Member

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?

Copy link
Member Author

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();
Copy link
Member

Choose a reason for hiding this comment

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

_whenDestroyed?

Copy link
Member

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.

Copy link
Member Author

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;
Copy link
Member

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?

Copy link
Member Author

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) {
Copy link
Member

Choose a reason for hiding this comment

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

markAsDestroyed ?

Copy link
Member Author

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) {
Copy link
Member

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.

Copy link
Member Author

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> {
Copy link
Member

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?

Copy link
Member Author

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.

@josephperrott josephperrott force-pushed the snack-bar branch 4 times, most recently from afc2320 to 91a3e3c Compare September 26, 2016 04:05
Complex: '375ms',
Entering: '225ms',
Exiting: '195ms',
};
Copy link
Member

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. */
Copy link
Member

@jelbourn jelbourn Sep 26, 2016

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.

Copy link
Member Author

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) {
Copy link
Member

Choose a reason for hiding this comment

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

_ngZone

Copy link
Member Author

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());
Copy link
Member

@jelbourn jelbourn Sep 26, 2016

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(() => { ... }); 

Copy link
Member Author

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();
Copy link
Member

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();
Copy link
Member

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()

Copy link
Member Author

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,
Copy link
Member

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.

Copy link
Member Author

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.

@josephperrott josephperrott force-pushed the snack-bar branch 3 times, most recently from a27b2fc to 7ec6fd3 Compare September 26, 2016 14:58
@josephperrott josephperrott force-pushed the snack-bar branch 2 times, most recently from 4aef5ea to 42febfb Compare October 3, 2016 16:49
@josephperrott josephperrott force-pushed the snack-bar branch 2 times, most recently from fa83858 to fbb2d4f Compare October 5, 2016 17:10
Copy link
Member

@jelbourn jelbourn left a 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)'; }
Copy link
Member

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)';

@jelbourn jelbourn merged commit 6df29dc into angular:master Oct 18, 2016
@josephperrott josephperrott deleted the snack-bar branch October 28, 2016 22:00
@andrewseguin
Copy link
Contributor

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:
snackBarRef.afterDismissed().subscribe(null, null, () => { expect(dismissed).toBeTruthy('Expected the snack bar to be dismissed'); expect(overlayContainerElement.childElementCount) .toBe(0, 'Expected the overlay container element to have no child elements'); });

@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 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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.

5 participants