Fix bug where removal of TeachingTip would leave the Popup open #1327
Conversation
Co-Authored-By: Jevan Saks <[email protected]>
Co-Authored-By: Jevan Saks <[email protected]>
Co-Authored-By: Jevan Saks <[email protected]>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@chingucoding can you add a test to cover this scenario? If you haven't run the tests before you can use the Visual Studio test explorer. I wouldn't recommend doing a "run all" but you can run specific teachingtip tests. There's probably some tests that already do something similar (like clicking outside to close a light dismiss) and so you would be able to do something like navigate backwards and then check that the teachingtip is no longer present in the tree. The test code is here: |
|
Unfortunately I won't be able to fix this today, but I will add a test for that over the weekend. |
No problem! Also I'd like to assign to you the issues that you plan to fix. Would you mind adding a comment in the issues you're picking off like "I'll fix this" -- adding the comment enables me to assign the issue to you. |
|
I have added the new test. In addition to this I also made sure that the unloading of the TeachingTip will set its open state to false. |
jevansaks
added
the
release note
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| @@ -15,6 +15,7 @@ TeachingTip::TeachingTip() | |||
| __RP_Marker_ClassById(RuntimeProfiler::ProfId_TeachingTip); | |||
| SetDefaultStyleKey(this); | |||
| EnsureProperties(); | |||
| Unloaded({ this, &TeachingTip::ClosePopupOnUnloadEvent }); | |||
There was a problem hiding this comment.
Unloaded({ this, &TeachingTip::ClosePopupOnUnloadEvent }); [](start = 4, length = 58)
I'm slightly concerned about how this will appear to behave to the developer. Teaching tips can be declared anywhere and I'm not positive under which circumstances the tip will be loaded (and thus unloaded). I did some experiments placing the teaching tip in different resource dictionaries and it behaves as I would expect, but I wonder if we haven't thought about all the scenarios.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
We have an open conversation about whether or not the closing animation should play in these scenarios, once we answer the question I'll merge this in or update the change. Additionally we want to have the same behavior when the target of the tip is unloaded, I've opened a new issue for that #1426. |
👍
Would this be something this PR should also cover? I think fixing #1426 would work in a very similar way. |
It doesn't have to be, but can be if you wanted to tackle that too :). We have decided that no animation is correct so this change is good. The added part for target would be to attach ClosePopupOnUnloadedEvent to the target's unloaded event (and detach when the target is changed or removed etc.) |
I can work on this but I might not be able to get to this soon. 😅 |
Happily, @chingucoding! Can you add a comment to that issue so i can assign it to you? (GitHub won't let me do it if you haven't commented). |
|
I have updated the OnPropertyChanged to add the |
dev/TeachingTip/InteractionTests/TeachingTipTestPageElements.cs
Outdated
Show resolved
Hide resolved
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
I'll merge once the CI gates come back. Thanks for doing this @chingucoding and your patients getting it in. |
Always happy to help! Thank you for your help 😄 |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
… into teachingtip-closepopupunload
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Summary
Fixed a bug where the removal of the TeachingTip from the visual tree (e.g. by navigating to a different page) would leave the popup open and present.
Motivation
Fixes #509 and fixes #1426
Testing methodology
Add a TeachingTip which was targeted to a page specific item and opened the TeachingTip popup. After that I navigated to a different page and checked that the popup is closed.