Skip to content

refactor(sidenav): switch to the angular animations API #4959

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

Merged
merged 8 commits into from
Jul 31, 2017

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Jun 4, 2017

Switches the sidenav to use the @angular/animations API. This saves us a lot of code that was involved in orchestrating the various animations, in addition to making it easier to follow. It also makes it possible for the consumer to disable the animations by using the NoopAnimationsModule.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jun 4, 2017
* rejected if it didn't). */
open(): Promise<MdSidenavToggleResult> {
/** Open the sidenav. */
open(): void {
return this.toggle(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

since these are all void now the return isn't necessary

*/
close(): Promise<MdSidenavToggleResult> {
/** Close the sidenav. */
close(): void {
return this.toggle(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

return not needed

return this._toggleAnimationPromise ||
Promise.resolve(new MdSidenavToggleResult(isOpen ? 'open' : 'close', true));
}
toggle(isOpen: boolean = !this.opened): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume the animation API provides some way for people to still hook into these events without returning the promise?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can have an observable that emits in the done callback, but I'm not sure that it's worth the trouble.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just asking since I don't know much about the animations API, how do I as a user who is currently doing this:

snav.toggle().then(() => doStuff())

change it to work after this PR? I would guess something like this?

<md-sidenav (@transform.start)="doStuff()"></md-sidenav>

Copy link
Member Author

@crisbeto crisbeto Jun 6, 2017

Choose a reason for hiding this comment

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

That won't work, because the Angular animation events don't propagate IIRC. The way to do it would be:

snav.onOpen.subscribe(() => doStuff());
snav.open();

I guess we could return the onOpen and onClose observables from the open/close methods, but I'm not sure whether it's necessary. The promises that were a part of the old API looked like they were there only there for synchronizing the animations internally.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok sg, just wanted to make sure we weren't taking away functionality

Copy link
Contributor

Choose a reason for hiding this comment

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

@crisbeto Let's add back these promises because they are used pretty extensively throughout Google's code. We can leave a comment to remove them in the future. (I can do so as part of my other upcoming breaking sidenav changes).

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.

@crisbeto
Copy link
Member Author

crisbeto commented Jun 5, 2017

Addressed the feedback @mmalerba.

@crisbeto crisbeto force-pushed the sidenav-refactor branch from 324ed27 to d958c8a Compare June 8, 2017 17:20
@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase action: merge The PR is ready for merge by the caretaker labels Jun 8, 2017
@kara kara added the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label Jun 12, 2017
@kara
Copy link
Contributor

kara commented Jun 12, 2017

Note: need to update some tests that rely on mat-sidenav-opening / mat-sidenav-closing classes before merging

@mmalerba mmalerba added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Jun 21, 2017
@mmalerba
Copy link
Contributor

Please rebase

@crisbeto
Copy link
Member Author

crisbeto commented Jun 21, 2017

Seems like the tests are failing after a rebase. Will look into it tomorrow.

@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Jun 24, 2017
@mmalerba mmalerba added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Jul 7, 2017
@mmalerba
Copy link
Contributor

mmalerba commented Jul 7, 2017

sorry, needs more rebasing

@crisbeto crisbeto force-pushed the sidenav-refactor branch from 32d12ff to aee2e86 Compare July 8, 2017 08:32
@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Jul 8, 2017
@kara
Copy link
Contributor

kara commented Jul 21, 2017

@crisbeto Lint is failing right now; can you update?

@kara kara removed the action: merge The PR is ready for merge by the caretaker label Jul 21, 2017
@kara kara assigned crisbeto and unassigned mmalerba Jul 21, 2017
@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Jul 21, 2017
@mmalerba mmalerba removed the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label Jul 25, 2017
@mmalerba
Copy link
Contributor

@andrewseguin Return values have been added back to toggle & friends, want to try another presubmit

@andrewseguin
Copy link
Contributor

Not sure what I'm seeing here, but it looks like you need to rebase. I see this in your sidenav.ts

private _watchSidenavToggle(sidenav: MdSidenav): void {
    if (!sidenav || sidenav.mode === 'side') { return; }
    sidenav.onOpen.subscribe(() => this._setContainerClass(true));
    sidenav.onClose.subscribe(() => this._setContainerClass(false));
  }

which differs from master:

private _watchSidenavToggle(sidenav: MdSidenav): void {
    merge(sidenav.onOpenStart, sidenav.onCloseStart).subscribe(() => {
      this._changeDetectorRef.markForCheck();
    });

    if (sidenav.mode !== 'side') {
      sidenav.onOpen.subscribe(() => this._setContainerClass(true));
      sidenav.onClose.subscribe(() => this._setContainerClass(false));
    }
  }

@crisbeto
Copy link
Member Author

Weird that GitHub didn't pick that one up as a conflict @andrewseguin. I've rebased it and fixed the issue.

@andrewseguin
Copy link
Contributor

Sorry one more thing, the e2e is getting timeouts on the CI. I tried re-running and got the same error. Can you check it out?

crisbeto added 7 commits July 29, 2017 11:13
Switches the sidenav to use the `@angular/animations` API. This saves us a lot of code that was involved in orchestrating the various animations, in addition to making it easier to follow. It also makes it possible for the consumer to disable the animations by using the `NoopAnimationsModule`.
@crisbeto
Copy link
Member Author

Sorted out all the failures @andrewseguin.

@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
action: merge The PR is ready for merge by the caretaker 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.

6 participants