TeachingTip: Fix HeroContentPlacement being ignored when initially set from XAML markup #3271
Conversation
dev/TeachingTip/TeachingTip.cpp
Outdated
| { | ||
| m_currentHeroContentEffectivePlacementMode = winrt::TeachingTipHeroContentPlacementMode::Top; | ||
| TeachingTipTestHooks::NotifyEffectiveHeroContentPlacementChanged(*this); | ||
| DoUpdateDynamicHeroContentPlacementToTop(); | ||
| } |
There was a problem hiding this comment.
I believe the no-opt was intentional, in the Auto case we do not want to reposition the hero content while the tip is open, so delay processing this change until the next time we open.
There was a problem hiding this comment.
That will lead to an issue where the VisualStateGroup "HeroContentPlacementStates" has no current state when the tip is launched in the Auto HeroContentPlacement mode. And according to the documentation, this should not happen: https://docs.microsoft.com/en-us/uwp/api/windows.ui.xaml.visualstategroup.currentstate?view=winrt-19041#remarks
Since we place the HeroContent at the top of an untargeted TeachingTip given a HeroContentPlacement of Auto we should put the VisualStateGroup in the HeroContentTop state here which is what DoUpdateDynamicHeroContentPlacementToTop() will do.
If an update of the HeroContentPlacement setting to Auto is still not desired while an untargeted TeachingTip is open I can add an additional check here to only call DoUpdateDynamicHeroContentPlacementToTop() when the tip is untargeted and unopened. Though I'm not sure why we would want to block that for untargeted TeachingTips, since in their case, the hero content will always be placed at the top of the tip and the tip itself does not have a tail we have to take into consideration here.
There was a problem hiding this comment.
the Auto scenario means the app author wants us to decide was is best, if they wanted the hero content to be top then they could set the the property to top. We no-opt here because we believe that switching the hero content position from bottom to top while the tip is open is usually a jarring experience so we delay the switch until the next opening. Why is the null state an issue for untargetted but not targetted tips. Can we change the the Untargetted opening logic to do the same thing the targetted one does with respects to actively choosing a hero content state?
In reply to: 487262633 [](ancestors = 487262633)
There was a problem hiding this comment.
Reverted this change (removed that call). An untargeted TeachingTip will now behave the same as a targeted TeachingTip regarding its HeroContentPlacement visual state group when in Auto hero content placement mode: Once the tip is open either the Top or Bottom visual state wil be entered. Before the tip has been opened, no state is applied (HeroContentPlacementStates.CurrentState = null).
…en changing the HeroContentPlacement to auto (while tip is open).
|
@StephenLPeters Addressed your concerns. Please take a look. |
|
/azp run |
1 similar comment
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
1 similar comment
|
Azure Pipelines successfully started running 1 pipeline(s). |
Description
This PR fixes an issue where the
HeroContentPlacementAPI was ignored when initially set from XAML markup.Motivation and Context
Fixes #3270.
How Has This Been Tested?
Tested manually and added API test.
Screenshots: