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

Conversation

codymikol
Copy link
Contributor

prevent interimElements from being tracked as showingInterimElements
when they fail to be compiled

Fixes: #11460

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?

If an interimElement fails to compile it will still be added as a "showingInterimElement"

Issue Number: #11460

What is the new behavior?

Now interimElements will only be tracked in the showingInterimElements list if they successfully compile.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Unfortunately the showingInterimElements var is private to the InterimElementFactory and so it is not possible to write unit tests for this without adding additional functionality to the source. If the maintainers decide that is necessary I can definitely do so.

@googlebot googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Oct 2, 2018
@Splaktar Splaktar self-requested a review October 11, 2018 05:09
@Splaktar Splaktar self-assigned this Oct 11, 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.

This looks reasonable, but we need a unit test to make sure that we don't break this again in the future.

Thank you!

@Splaktar Splaktar added in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: unit tests This PR needs unit tests to cover the changes being proposed P3: important Important issues that really should be fixed when possible. labels Oct 11, 2018
@Splaktar Splaktar added this to the 1.1.12 milestone Oct 11, 2018
Copy link
Contributor Author

@codymikol codymikol left a comment

Choose a reason for hiding this comment

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

@Splaktar any updates on my above comment?

edit: Not sure why this came through as a review instead of a comment and now it says my above comment was made just now when It was written quite a while ago 🤷‍♂️

@Splaktar Splaktar modified the milestones: 1.1.12, 1.1.13 Jan 3, 2019
@Splaktar Splaktar modified the milestones: 1.1.13, 1.1.14 Feb 10, 2019
@codymikol codymikol force-pushed the fix-interim-nonsense branch 2 times, most recently from 078708c to a27e312 Compare February 19, 2019 05:06
@codymikol
Copy link
Contributor Author

@Splaktar I finally got around to adding a test for this. I verified that the test fails in master and passes in this branch.

prevent interimElements from being tracked as showingInterimElements
when they fail to be compiled

Fixes: angular#11460
@codymikol codymikol force-pushed the fix-interim-nonsense branch from a27e312 to dea2ab9 Compare February 19, 2019 05:08
@codymikol
Copy link
Contributor Author

@Splaktar any update on this? 🐔

@Splaktar Splaktar added pr: merge ready This PR is ready for a caretaker to review pr: lgtm This PR has been approved by the reviewer type: bug and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: unit tests This PR needs unit tests to cover the changes being proposed labels Mar 14, 2019
@Splaktar
Copy link
Contributor

Sorry for the delay on this, it looks good to go! Thank you

@mmalerba mmalerba merged commit 08d90e9 into angular:master Mar 18, 2019
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/ P3: important Important issues that really should be fixed when possible. pr: lgtm This PR has been approved by the reviewer 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.

dialog: "Ghost Dialogs" created when $mdDialog resolver fails
4 participants