microsoft-ui-xaml
microsoft-ui-xaml copied to clipboard
microsoft
NavigationView: Fix issue with MenuTemplate interfering with FooterItems by introducing FooterMenuItemsTemplate
API spec
The API spec is #4518
Description
When using FooterItems with MenuItemTemplate, the datatemplate interferes resulting in missing animations. Because of that, the footer repeaters no longer will receive the NavViewFactory as they don't need it.
This PR also introduces the FooterMenuItemsTemplate and FooterMenuItemsTemplateSelector property.
The issue #3793 is purely animation wise, so any automated test would have to walk the visual tree which is why there currently is not one in this PR as those tend to break easily.
Motivation and Context
Closes #3793
How Has This Been Tested?
Tested manually, add small interaction test
Would this cause the MenuItemTemplate to not apply for the Footer Items? That'd be nice - I personally have a different behaviour for these items, and I think that's also a common thing to happen.
Just a thought to add to this: If someone actually needs the template to apply, might it be sensible to add a F FooterItemTemplate as parameter maybe, since we already have a FooterMenuItemsSource anyways? And if somebody needed the same behaviour on both the main MenuItems and the Footer Items, putting the datatemplate into a dictionary and using it as static resource on both menu and footer would handle that.
Given that 2.5 does apply the MenuItemTemplate, I don't think that should be changed to not do this anymore.
Would this cause the MenuItemTemplate to not apply for the Footer Items? That'd be nice - I personally have a different behaviour for these items, and I think that's also a common thing to happen.
Sorry about the delay @Shad0wlife, yes this would result in the footer item template to not be applied to the footer collections. I'm onboard with you here, footer items generally have a different behavior and having the template applied to both collections might be misleading/interfering devs.
Given that 2.5 does apply the MenuItemTemplate, I don't think that should be changed to not do this anymore.
The spec does not imply that this should be the case though so it was rather an oversight I would argue.
@anawishnoff FYI
Not having the option for a template seems broken though as then there is no good way for FooterMenuItemsSource to work.
@chingucoding @YuliKl @anawishnoff @StephenLPeters This sounds like we are breaking the existing behavior. If i understand correctly, the footer items are getting the same template as the menu items, which might be by design. Does the template selector also work the same way ? If that scenario is broken, can we get that to work instead of not using the template? The other alternative I can think of is to have a separate FooterItemTemplate specific for footers
Because the FooterMenuItems list consists of regular NavigationViewItem objects, FooterMenu items should have the MenuItemTemplate applied to them, the same as the MenuItems. I would think that if MenuItemTemplateSelector is set, it should also apply/not apply to FooterMenu items in the same way that it would apply/not apply to regular MenuItems.
However I understand that this may not be the desired behavior as some people want their footer menu items to look and act different from other menu items. Although I think adding a FooterItemMenuTemplate sounds like a good idea, why not just manually use ContentTemplate to edit the look of your FooterMenuItem? Is that not recommended?
I think there is a benefit having separate template. As @Shad0wlife, there are cases where you don't want to have the tempalte be applied to the footer items. I think a key problem could be that if the template is being applied, we might run into issues á la "wrong type" since devs might want to use different type of objects for the menu source and the footers source.
@chingucoding @Shad0wlife - ahh, good points. I see what the issue is now. If we want to stop the MenuItemTemplate from being applied to footer menu items, I think the best solution would be to have a separate FooterMenuItemTemplate so that those items can be easily customized as a unit.
So what would the next steps be? Merge this and introduce FooterItemTemplate in a separate PR? Or should we do this in this one?
So, I think I just encountered issues using FooterMenuItems when having a MenuItemTemplate set in the WCT sample app.
This causes two major issues for us in my PR https://github.com/windows-toolkit/WindowsCommunityToolkit/pull/3824:
-
ItemInvokedreturnednullfor theInvokedItemwhen clicking on aFooterMenuItemin this setup - Providing our own
NavigationViewItemin XAML was applying theMenuItemTemplateand effecting the rendering of it because it was forced to use the sameMenuItemTemplateas the main items.
Typically, I don't expect a dev to have the same MenuItemTemplate for the regular items and footer items, as I'd expect to use MenuItemsSource for the main ones and custom specify the footer ones.
If there's a FooterMenuItemsSource then there should be a separate FooterMenuItemTemplate to go with it as you may have different sources for the different collections requiring two different templates.
It's really bizarre to share the template here.
@chingucoding It would be great for you to introduce FooterMenuItemTemplate in this PR if you'd like to! Calling on @ranjeshj to see if there's any logistical issues with that.
@chingucoding It would be great for you to introduce FooterMenuItemTemplate in this PR if you'd like to! Calling on @ranjeshj to see if there's any logistical issues with that.
@anawishnoff I think this makes sense, but it is a new API addition, so we need to follow the usual process for that.
Sure @anawishnoff and @ranjeshj, I'll add it to this PR as a preview API. @anawishnoff would you like to create the appropriate PR in the specs repo or even in here? Otherwise, I can create the PR once I added it here.
@anawishnoff @ranjeshj Footer items template is now implemented in this PR.
@chingucoding Thanks so much Marcel! Sorry about the delayed response on my end. I think we're moving to create specs on this repo from now on. I can start on a PR for this and lay the foundation for the spec. If you're able and would like to, we can work together on adding more info on the API from your side. I'll come back and comment here once I've opened the PR.
Thanks a lot @chingucoding! I've opened up a PR with a first draft of the spec here: https://github.com/microsoft/microsoft-ui-xaml/pull/4518
Feel free to comment, review, and make any necessary changes based on the work you've done.
I'm still interested in progress on this PR and it's been a while since there was any news on this, and neither is there any news on the Spec PR. Is the issue here the missing check on both PRs?
I too just hit this again, really frustrating to not be able to use FooterMenuItems of static XAML defined NavigationViewItems with an ItemsSource of menu items. They're two independent purposes... It's really weird to think that NavigationView expects everything to be a data type item and use the same template when you go down that route. This really seems to break from how XAML works. And if your hierarchical data is of two different types for nodes/children it's forces you down a complex path pretty quickly vs. NavigationViewItem having it's own template property for its children as well.
Thanks for implementing this @chingucoding! I'm closing this PR because it does not meet the bar for WinUI2, however I will work on moving your change into WinUI3.
@ojhad If you're closing this, you might want to check if #4518 needs to be reverted. That's the corresponding API spec that has already been merged.
Thanks for implementing this @chingucoding! I'm closing this PR because it does not meet the bar for WinUI2, however I will work on moving your change into WinUI3.
It's going to be really hard if there's a fix for WinUI 2 and it's only patched in WinUI 3 to keep code-bases that are multi-targeting both frameworks in-sync with the proper behavior.
It's going to be really hard if there's a fix for WinUI 2 and it's only patched in WinUI 3 to keep code-bases that are multi-targeting both frameworks in-sync with the proper behavior.
@michael-hawker There are several fixes/features in WinUI3 that do not exist in WinUI2. Given that the bar for taking fixes in WinUI2 is very high and new features/fixes are being made in WinUI3, you will need some way to light up in WinUI3 and gracefully degrade when using WinUI2.
@ojhad If you're closing this, you might want to check if #4518 needs to be reverted. That's the corresponding API spec that has already been merged.
@Shad0wlife, the spec is still useful. We would need to document this for WinUI3.