Skip to content

Conversation

@sv2dev
Copy link

@sv2dev sv2dev commented May 29, 2017

Fixes #4859

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@googlebot googlebot added the cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla label May 29, 2017
@sv2dev
Copy link
Author

sv2dev commented May 29, 2017

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels May 29, 2017
@sv2dev
Copy link
Author

sv2dev commented Jun 12, 2017

@jelbourn from my point of view this is ready to review.

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.

This change needs a unit test that verifies the problem it aims to resolve no longer occurs.

private _onAction: Subject<any> = new Subject();

/** Timeout handle for the dismiss timeout cleanup. */
private _dismissTimeoutHandle: number;
Copy link
Member

Choose a reason for hiding this comment

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

I would call this durationTimeoutId with comment:

/**
 * Timeout ID for the duration setTimeout call. Used to clear the timeout if the snackbar is 
 * dismissed before the duration passes.
 */
private _durationTimeoutId: number;

}

/** Dismisses the snack bar after some duration */
dismissAfter(duration: number): void {
Copy link
Member

Choose a reason for hiding this comment

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

This should be an private method. If it were a public API and someone called it in combination with setting a duration, the earlier timeout ID would be lost and the issue this PR aims to resolve would still occur.

@sv2dev
Copy link
Author

sv2dev commented Jun 19, 2017

Hi @jelbourn,

Thanks for reveiw.

I did the requested changes, but the tests seem to fail, since flush() (introduced in 4.2.0-rc.1) is missing in CI.
I'm not sure why this is the case.
I first thought, that the rc version of angular 4 in the yarn.lock might be the issue, but the travis job uses npm to install the dependencies.
When I run npm i, I get @angular/[email protected] installed and it has the flush() function.

Do you have any idea how to fix this or test the behavior differently?

@jelbourn
Copy link
Member

@svi3c

  it('should clear the dismiss timeout when dismissed before timeout expiration', fakeAsync(() => {
    let config = new MdSnackBarConfig();
    config.duration = 1000;
    snackBar.open('content', 'test', config);

    setTimeout(() => snackBar.dismiss(), 500);

    tick(600);
    viewContainerFixture.detectChanges();
    flushMicrotasks();

    expect(viewContainerFixture.isStable()).toBe(true);
  }));

@sv2dev
Copy link
Author

sv2dev commented Jun 21, 2017

Hi @jelbourn,
the changes are applied.

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

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs cleanup / tests labels Jun 21, 2017
Sven Reglitzki and others added 2 commits June 23, 2017 13:17
When a duration is passed to MdSnackBar.prototype.openFromComponent(),
a timeout is created. This timeout is now cleared when dismissing the
snackbar.

Fixes angular#4859
@jelbourn jelbourn merged commit 146160c into angular:master Jun 23, 2017
@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.

SnackBar does not close zone when dismissed

3 participants