Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

fix(toast): remove the interim element from the list before closing element #11427

Merged

Conversation

marosoft
Copy link
Contributor

Check if the interim element is valid before proceeding.

Fixes #10715

PR Checklist

Please check that your PR fulfills the following requirements:

  • The commit message follows our guidelines
  • Tests for the changes have been added or this is not a bug fix / enhancement
  • Docs have been added, updated, or were not required

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Enhancement
[ ] Documentation content changes
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

At the moment the interim element is being removed from the list after new hideAction promise is created for it. In addition no extra check is made for the interim object before it is used.
This is causing an error as undefined object is trying be accessed.
Issue Number: #10715

What is the new behavior?

The interim element is removed from the list before the closeElement method is called.
An extra check is added at the beginning of that method to make sure the interim element is a valid object.

Does this PR introduce a breaking change?

[ ] Yes
[X] No

Other information

…lement

Check if the interim element is valid before proceeding.

Fixes angular#10715
@googlebot googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Aug 26, 2018
@Splaktar Splaktar self-assigned this Aug 29, 2018
@Splaktar Splaktar self-requested a review August 29, 2018 01:04
Copy link
Contributor

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

This looks good to me.

Can you please add a simple unit test that verifies this fix?

@Splaktar Splaktar added this to the 1.1.11 milestone Aug 29, 2018
@Splaktar Splaktar added needs: unit tests This PR needs unit tests to cover the changes being proposed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs type: bug g3: reported The issue was reported by an internal or external product team. P2: required Issues that must be fixed. labels Aug 29, 2018
@Splaktar
Copy link
Contributor

@marosoft I'd like to get this into 1.1.11 which I hope to release soon. Can you please add a quick unit test for this so that I can get it merged?

@Splaktar Splaktar added pr: merge ready This PR is ready for a caretaker to review and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Sep 25, 2018
@marosoft
Copy link
Contributor Author

@Splaktar sorry for a delay. I tried adding a small unit test in the interim element component but I couldn't reproduce the issue. I am not that much familiar with the internals and I might doing something wrong. I will try to spend some time tomorrow on it.

@Splaktar Splaktar removed the needs: unit tests This PR needs unit tests to cover the changes being proposed label Oct 17, 2018
Copy link
Contributor

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

I am also unable to reproduce this issue with a unit test. I tried an $mdToast test that looks just like the original issue's CodePen, but it doesn't throw an exception.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ g3: reported The issue was reported by an internal or external product team. P2: required Issues that must be fixed. pr: merge ready This PR is ready for a caretaker to review type: bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

toast: multiple show()/hide() causes TypeError
6 participants