Windowstemplatestudio: Update the Shell Page of the NavPanel to use the new NavigationView control

[mrlacey]

Use the new native control when targeting Fall Creators Update

https://docs.microsoft.com/en-us/windows/uwp/controls-and-patterns/navigationview

[crutkas]

goal here is to use the UWP Community Toolkit control, they will do the heavy lifting to support it, we migrate off once we have mainstream Fall Creators Update support. We won't migrate to this for a bit.

[crutkas]

cross ref: #212

[mrlacey]

Yes, there was a PR somewhere for moving to the community toolkit version. Added this when I discovered that the FCU will have a native control. At that point, we should switch to the native control, from the toolkit.

[crutkas]

agreed but that won't be for a bit due to the n-1 release rule.

[mrlacey]

@crutkas Xref #334 would be good to understand the implications of the N-1 rule. It also stops the ability to use WTS to explore new SDKs when they are out in preview. It's also the cause of #327. If we can get stats on the distribution of what versions people are running it would really inform this decision.

[crutkas]

This will happen after next release after Fall Creators Update

[ralarcon]

@mrlacey @crutkas We need to evaluate the implications of this change as well as if we can aford it for 1.5

[crutkas]

we are not updating to NavView until the next full update of Windows. The UWP Toolkit will handle a smart flip for this control depending on the build you are on.

[crutkas]

This will happen in the spring 2018 time frame :)

[mvegaca]

There is a documentation to explain how to move to NavigationView from HamburguerControl.

[mvegaca]

I will have a look on that.

[mrlacey]

The toolkit documentation is something we should point anyone with an existing app too.

Need to consider the impact on Settings in particular and try our best to not break backward compatibility when using 2.x templates to add pages or features to projects created with 1.x.

[mvegaca]

I've been working on adding NavigationView control to SplitView projects. In this branch can be generated apps that uses NavigationView control with all project types, Mvvm Basic framework, and Blank-Settings pages.

The first approach done is based on NavigationView documentation. We were trying to not use the Switch sentence on ShellViewModel and after finding this article on Jerry Nixon we update the templates to generate the apps using an Extension Class.

CountriesApp_Switch.zip
CountriesApp_Extension.zip

Templates update summary:

• Uwp Community Toolkit stops being necessary on Navigation pane projects.
• Now we don't need the ShellNavigationItem class.
• Titles in pages are now contained in the NavigationView HeaderTemplate instead of pages code. That provides a best user experience. In other project types (Blank and Pivot) The title of pages is composed using a composition template.
• SettingsPages is added to the NavigationView control using the property IsSettingsVisible and handle settings NavigationViewItem item in code. The problem here is that NavigationView has the responsibility to set the icon and the content of this menu item and always set the Gear icon and Settings label (this label is translated to other languages on runtime). If we use that we could think on cancel settings page name edition in the wizard.
• Now if you add a page to an old project that uses HamburgerMenu the postaction that adds the NavigationViewItem fails. It can be explained on a documentation with two options: How to add the new generated page to an old HamburguerControl or how to Update HamburguerControl on your project to start using NavigationView control.

@mrlacey @crutkas could you please review the two different approaches?

IMO Extensions created by Jerry Nixon is a better solution but could be not easier to understand to a new developer in the platform, but this removes a postaction on ShellViewModel and you can not break the navigation by changing a Tag property.

[mvegaca]

Documentation for How to Update from HamburgerMenu to NavigationView on PR #2084 Issue #2082.
Documentation file in PR origin.

• updatetonavigationview.md

[mrlacey]

The documentation provided on how to manually update an existing project needs to match the way that future projects will be produced to enable manually updated projects to work with future versions of the tool and its templates.
In that the way that the tool will generate projects in the future is not yet decided, it's not possible to provide equivalent documentation. Any documentation provided now could lead to future problems.

Here are my issues with the initial version of the documentation.

• It only covers projects built with the MVVMBasic framework. The code in projects built with other frameworks has substantial differences, and it's not appropriate to assume that someone with a project using another framework will be able to convert their project based on the instructions provided.
• The document does not explain how existing menu items correspond to new NavigationMenuItems.
• By creating NavigationMenuItems directly in the XAML, the logical grouping of what is displayed in a menu item and what happens when it's invoked is now lost. This was previously contained in ShellNavigationItem.
• The use of NavigationViewItemExtensions seems to have been inspired by an MSDN magazine article. That article uses this approach for specifying the target of navigation as part of a goal of removing an unnecessary view model and doing the work in code behind to break the coupling between the ViewModel and the view. The documented approach here mixes a combination of code in the "code behind" file and a view model. Mixing the location of related coded increases the complexity and the coupling between layers.
• The document does not allow for scenarios where the menu item may do something other than trigger navigation. (such as is documented here.)
• Using the NavigationView's Header property for every page may not be what is wanted on every page. Using this approach of always showing the header makes it much harder for anyone with a page where they don't want the title header displayed to remove it. If it were left on the page, it would be easier to include it or not. Moving the title out of the page when using a NavPane project also means the same page template can't be used for each project type which leads to increased template complexity. (assuming that this is a guide for future templates.)
• Requiring the modification of every page in the app (to remove the title/header and adaptive triggers) seems an onerous request. Is it really necessary?
• Should add a warning about removing the adaptive triggers as the developer may also be using them for something else.

• The same result as that detailed by the document could have been achieved by just modifying the ShellPage XAML and the related code in the ViewModel (or equivalent) so that the original items collection is used. No need to add new classes and no need to remove any classes. No need to break backward compatibility with existing templates (apart from SettingsPage). - This may not be the best solution if we didn't have legacy apps to consider, but this has highlighted some inadequacies in the NavigationView control which mean that there's no perfect solution here.

• For a manual upgrade process, I think we need a really good argument for anything other than the simplest solution that requires the minimum of effort.

• We also need to consider the possibility of any perceived consequences of not using the control as it comes. By encouraging the use of extensions to provide basic functionality to the app are we implying that it's not good enough as it is? If that's the perception, we need to be aware of it. If that's the case, have these concerns been communicated to the team behind the control?

[sibille]

@mrlacey, You're raising good points!

Documentation is not finished yet, we just wanted to get it started, we'll have to adapt to what we decide is the final solution. You're right that it should include all frameworks.
Let's decide on implementation first, then adapt documentation.

From the points you listed I understand that you propose to implement NavigationView using the ShellNaviagtion Item. Is that assumption correct? We initially did not explore this option as we wanted to do an implementation closer to what is recommended in the docs. But as with the extension I think it's a valid approach. We'll prepare a Poc of this approach and share.
I think making migration easier is a point in favor of that implementation, but I also believe we should choose what we think is the best implementation and go for that, and help people update their project to this state. (I opened an issue to work on that x-ref: #2086 )

Regarding the header: NavigationView does not allow to completely remove the header, even if you put it to AlwaysShowHeader it will show in narrow mode. If the page has a header too, both are shown, which results a bit strange. I think we should follow the guidance here and provide pages without header for NavView projects.

Regarding invocation of actions the guidelines of NavView states that NavView should not invoke actions, we might want to remove the document you mention.

[sibille]

Adding CountriesApp_ShellNavigationItem.zip
@mrlacey, was that what you were refering to?

[mrlacey]

@sibille yes something more like the ShellNavigation version of the country app. It would need a bit more tweaking for correctly highlighting the selected item correctly though.

It is possible to remove (or at least hide) the header, even when the navigationView is in the Minimal display mode. It just requires re-templating. But I agree, we probably shouldn't encourge this.
In terms of what we produce, we should reflect the intention of the creators of the control but we also need to think about explaining why widespread changes are being suggested as part of the migration.

The docs about triggering an action rather than navigating were based on secondary items. In the NavigationView equivalent for these is contained in the PaneFooter, it makes sense not to support these in the same way and instead let people put whatever they want in the footer.

I guess the important thing here is being clear about how the NavigationView has some fundamental differences and expectations of use compared to the HamburgerMenu. One is not necessarily a direct drop-in replacement for the other.

In terms of how different frameworks are implemented, we may also want to make some of the differences larger. For instance, the current "CodedBehind" method pretty much works the same as the MVVMBasic one but with the functionality from the ViewModel moved into the xaml.cs file. For the updated shell page that uses the NavigationView, I think it would make sense to make these more different and more reflect the sample code in the official documentation. (Assuming we can make such a change without breaking everything.)

How things are changed needs to be done with a mind on how backward compatibility and future changes to templates too.

Exploration continues...

[sibille]

@mrlacey, I reviewed the project, I think it's highlighting correctly the selected item. Is there anything I miss?

We'll remove titles from the pages and adapt documentation to include the points you mentioned.
We could point to docs for people that do not want to remove title.

We've 3 options to implement NavView, lets see advantages of each solution and decide on an implementation:

1) NavigationView with NavigationViewItem on MenuItems (CountriesApp_Switch)
- Implementation described on NavigationView docs
- Most basic way to add an NavigationViewItem (Icon + Label)
- Allows to add NavigationViewItemSeparators
 
2) NavigationView with NavigationViewItem on MenuItems + NavigationViewItemExtension (CountriesApp_Extensions)
- No switch statements in code behind.
- NavigationViewItems added in Xaml as shown in docs
- Allows to add NavigationViewItemSeparators
- No need for switch in code behind

3) ShellNavigationItem (CountriesApp_ShellNavigationItem)
- Makes migration of legacy projects easier
- Causes breaking changes only for SettingsPage
- No need for switch in code behind

@mrlacey: Feel free to add any points I might have missed.

We are not sure if making migration easier justifies the level of indirection ShellNavigationItem and DataTemplate that solution 3 is adding.

I think what a developer would choose depends on the size and complexity of the application they are developing.

Regarding if we could chose one model for CodeBehind and another for MVVM, we'll have a look at it, but I think it's possible.

@crutkas, what do you think, which way should we go?

[mrlacey]

@sibille the ShellNavigationItem sample doesn't handle the settings page when navigating back through the page stack. That's the issue I was referring to.

I think it's also worth mentioning the negatives of each option too.

Switch

• Uses duplicated magic strings that must match in the XAML & code files. This is a bad practice and should not be encouraged.
• It splits the definition of the items and the logic for their handling.
• Makes migration harder.
• Will make supporting old and new projects with new templates harder.

Extensions

• Feels like an admission that the default functionality of the control is inadequate.
• Makes migration harder.
• Will make supporting old and new projects with new templates harder.

ShellNavigationItem

• Less obvious how the code works than items added in XAML.
• Makes it hard to add item headers or separators.
• Unlikely to be reflective of how others use the control.

For completion, there's also a 4th option. That would be to use the current item collection and populate the menu items in code, which is what the HamburgerMenu does internally.
The positive argument for this approach is that it makes migration really simple.
The negative arguments are that it adds complexity and isn't likely to be obvious to people looking at the code.

---

When looking at the pros and cons of each option, the "extension" option stands out as the most appropriate and the one I think we should use.
We'll need to be very clear on the messaging around why and how things need to change as part of the migration documentation. There are also some tweaks to the naming from the sample that I'd like to make too, but that can be discussed later.

I _REALLY_ don't want to set a precedent that the introduction of breaking changes to templates requires manual changes to previously generated projects to be able to support adding pages or features to the existing project, but in this case, I suspect it will be necessary to avoid needing multiple versions of templates.
Let's see what the consequences are as we make the transition to the new control, but if we can easily avoid breaking any legacy updates that'd be good.

[sibille]

So, we're going with the extensions option, correct?
@mrlacey, which are the naming tweaks?

Regarding breaking changes I agree with you, but I think in this case it is unavoidable to break sth here and also we want people to move to Navview, as Hamburger menu is getting obsolete.

We're working on a system that detects if you're still using hamburger menu and shows you a notification to the docs to update. (X-Ref #2086)

We'll also have to work on improving the documentation to make upgrade easy and resolve possible doubts.

[mrlacey]

Regarding naming tweaks:

• Talk is about this being an "extension" approach but actually uses "attached properties" rather than "extension methods".
• I've previously only ever seen a directory called "extensions" contain classes containing extension methods or extended versions of other classes (that inherit and extend). I think what is currently called NavigationViewItemExtensions should go in the "Helpers" directory to better follow common conventions.
• NavigationViewItemExtensions while being specific is a long name (this is especially noticeable in the XAML) could something shorter be used? What about NavHelper?
• Methods in NavigationViewItemExtensions use the variable name obj for an item that is a NavigationViewItem. "obj" is more commonly used for variables of the type Object. I suggest changing the variable name to item to better indicate that it is the NavigationViewItem and not just a variable of a much less specific type.
• The property name PageType is very generic and not as descriptive of its purpose and use. What about calling it NavigateTo so that it is clearer that it is the Page that will be navigated to?
• The namespace alias could be more succinct too.

Consider these to variants of the same entry. (original, followed by my proposal.)

<NavigationViewItem x:Uid="Shell_Italy" Icon="Document" extensions:NavigationViewItemExtensions.PageType="views:ItalyPage" />

<NavigationViewItem x:Uid="Shell_Italy" Icon="Document" ext:NavHelper.NavigateTo="views:ItalyPage" />

[mvegaca]

I'm agree with your naming proposal Matt.
By now, NavigationView is added on C#: CodeBehind, MVVM Basic, MVVM Light and CaliburnMicro (Prism missing). Apps can be generated in this branch.
I also have the proposal to remove the title in page for TabbedPivot project as it looks redundant.

[mvegaca]

Title TextBlock is defined in a composition template with the filter ProjectType != NavigationView. We only need to update this to ProjectType = Blank.

[mrlacey]

I think we can remove the "Title TextBlock" from every page regardless of project type. It's basically just placeholder content and expected to be removed anyway.

[sibille]

Thats true, but the blank project would be very blank without the title I guess. It also provides guidance about how to localize strings

[mrlacey]

Actively working on the VB versions

[mvegaca]

VB template merged. I was checking that all works fine. I'm going to remove the titles on Pivot Projects on a separated commit.

[mvegaca]

On this commit I've updated the ItemNameEditable property to enable pages to be single instance template. This is oriented to set a fixed name to settings page because NavigationView isn't thought to have more than one settings page.

[sibille]

NavigationView is setting the settings page name always to "Settings" (or its localized values) independently of the name you choose, that's why we think settings page name should not be configurable in the wizard to avoid confusion. But we loose the possibility to rename it on the Tabbed/Pivot (and Blank) project type too.

Telem for the last month has been 70% SplitView (15% each Blank and Tabbed Pivot)

[mvegaca]

Pull Request added! #2115

[sibille]

Created #2126 to see if we can re-enable name editing for settings page in project type blank and tabbedpivot.

[sibille]

NavigationView is in dev now. @lee or @milazzom could you have a look at the Prism implementation. @nigel-sampson could you have a look at the Caliburn.Micro implementation. Thanks!!

[nigel-sampson]

Will take a look tonight thanks @sibille

[sibille]

@mrlacey, for CodeBehind the helper method that looks for the correct NavigationViewItem comparing NavHelper and SourcePage type is called IsNavHelperForPageType, on the other frameworks IsNavigationViewItemFromPageType. Is there a reason for naming this different in CodeBehind? I think both are correct names for all frameworks, but both are difficult to understand. Would IsMenuItemForPageType be a better name?

[mrlacey]

@sibille yes IsMenuItemForPageType does sound better.

[mrlacey]

I've raised a PR to standardize the names

[nigel-sampson]

Looks ok to me, a little sad to see the move away from a more view model driven approach and having the view "leak" into the view model as raised earlier by @mrlacey but some of the binding limitations on navigation view make this unfeasible.

There could be some ways to work around this given we're already abstracting the view behind an interface though.

[mrlacey]

Verified in
wizardVersion="v0.17.18102.1"
templatesVersion="v0.17.18102.3"