New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update the Shell Page of the NavPanel to use the new NavigationView control #308

Closed
mrlacey opened this Issue May 11, 2017 · 39 comments

Comments

Projects
None yet
6 participants
@mrlacey
Collaborator

mrlacey commented May 11, 2017

Use the new native control when targeting Fall Creators Update

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

@crutkas

This comment has been minimized.

Member

crutkas commented May 13, 2017

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

This comment has been minimized.

Member

crutkas commented May 13, 2017

cross ref: #212

@mrlacey

This comment has been minimized.

Collaborator

mrlacey commented May 16, 2017

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

This comment has been minimized.

Member

crutkas commented May 16, 2017

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

@mrlacey

This comment has been minimized.

Collaborator

mrlacey commented May 16, 2017

@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 comment has been minimized.

Member

crutkas commented Sep 19, 2017

This will happen after next release after Fall Creators Update

@mrlacey mrlacey modified the milestones: Backlog, 1.5 Sep 19, 2017

@ralarcon

This comment has been minimized.

Contributor

ralarcon commented Oct 27, 2017

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

@crutkas

This comment has been minimized.

Member

crutkas commented Oct 27, 2017

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 crutkas modified the milestones: 1.5, Backlog Oct 27, 2017

@crutkas

This comment has been minimized.

Member

crutkas commented Dec 20, 2017

This will happen in the spring 2018 time frame :)

@mvegaca

This comment has been minimized.

Member

mvegaca commented Mar 9, 2018

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

@mvegaca mvegaca self-assigned this Mar 9, 2018

@mvegaca

This comment has been minimized.

Member

mvegaca commented Mar 9, 2018

I will have a look on that.

@mvegaca mvegaca added the in-progress label Mar 13, 2018

@mrlacey

This comment has been minimized.

Collaborator

mrlacey commented Mar 14, 2018

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

This comment has been minimized.

Member

mvegaca commented Mar 14, 2018

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.

image

@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.

@crutkas crutkas modified the milestones: Backlog, 2.0 Mar 16, 2018

@mrlacey

This comment has been minimized.

Collaborator

mrlacey commented Mar 23, 2018

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

This comment has been minimized.

Member

mvegaca commented Mar 23, 2018

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.

image

@mvegaca

This comment has been minimized.

Member

mvegaca commented Mar 23, 2018

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

@mrlacey

This comment has been minimized.

Collaborator

mrlacey commented Mar 23, 2018

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

This comment has been minimized.

Member

sibille commented Mar 23, 2018

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 mrlacey self-assigned this Mar 27, 2018

@mrlacey

This comment has been minimized.

Collaborator

mrlacey commented Mar 27, 2018

Actively working on the VB versions

@mvegaca

This comment has been minimized.

Member

mvegaca commented Apr 2, 2018

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

@sibille sibille modified the milestones: 2.1, 2.0 Apr 3, 2018

@mvegaca

This comment has been minimized.

Member

mvegaca commented Apr 3, 2018

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.

image

@sibille

This comment has been minimized.

Member

sibille commented Apr 3, 2018

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

This comment has been minimized.

Member

mvegaca commented Apr 3, 2018

Pull Request added! #2115

@sibille

This comment has been minimized.

Member

sibille commented Apr 4, 2018

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

@sibille

This comment has been minimized.

Member

sibille commented Apr 6, 2018

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

This comment has been minimized.

Collaborator

nigel-sampson commented Apr 9, 2018

Will take a look tonight thanks @sibille

@sibille

This comment has been minimized.

Member

sibille commented Apr 9, 2018

@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

This comment has been minimized.

Collaborator

mrlacey commented Apr 9, 2018

@sibille yes IsMenuItemForPageType does sound better.

mrlacey added a commit to mrlacey/WindowsTemplateStudio that referenced this issue Apr 9, 2018

@mrlacey

This comment has been minimized.

Collaborator

mrlacey commented Apr 9, 2018

I've raised a PR to standardize the names

@nigel-sampson

This comment has been minimized.

Collaborator

nigel-sampson commented Apr 10, 2018

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.

@sibille sibille modified the milestones: 2.0, 2.1 Apr 10, 2018

@mrlacey

This comment has been minimized.

Collaborator

mrlacey commented Apr 12, 2018

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

@mrlacey mrlacey closed this Apr 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment