Skip to content
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
Closed
Assignees
Labels
Can Close Out Soon Work relating to this issue has been completed. enhancement Indicates the issue is a suggestion for an improvement or alteration to something that already exist fall-creators-update The issue relates to functionality released as part of the Fall Creators Update release of Windows 1
Milestone

Comments

@mrlacey
Copy link
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

@mrlacey mrlacey added the fall-creators-update The issue relates to functionality released as part of the Fall Creators Update release of Windows 1 label May 12, 2017
@crutkas
Copy link
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
Copy link
Member

crutkas commented May 13, 2017

cross ref: #212

@mrlacey
Copy link
Collaborator Author

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
Copy link
Member

crutkas commented May 16, 2017

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

@mrlacey
Copy link
Collaborator Author

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
Copy link
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
Copy link
Contributor

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

@crutkas
Copy link
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 crutkas added enhancement Indicates the issue is a suggestion for an improvement or alteration to something that already exist and removed fall-creators-update The issue relates to functionality released as part of the Fall Creators Update release of Windows 1 labels Oct 27, 2017
@crutkas
Copy link
Member

crutkas commented Dec 20, 2017

This will happen in the spring 2018 time frame :)

@sibille sibille added the fall-creators-update The issue relates to functionality released as part of the Fall Creators Update release of Windows 1 label Mar 2, 2018
@mvegaca
Copy link
Collaborator

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
Copy link
Collaborator

mvegaca commented Mar 9, 2018

I will have a look on that.

@mvegaca mvegaca added the in-progress The issue is currently being actively worked on. label Mar 13, 2018
@mrlacey
Copy link
Collaborator Author

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
Copy link
Collaborator

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
@mvegaca
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator

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
Copy link
Collaborator Author

mrlacey commented Mar 27, 2018

Actively working on the VB versions

@mvegaca
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

mvegaca commented Apr 3, 2018

Pull Request added! #2115

@mvegaca mvegaca removed the in-progress The issue is currently being actively worked on. label Apr 3, 2018
@sibille
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

Will take a look tonight thanks @sibille

@sibille
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

mrlacey commented Apr 9, 2018

I've raised a PR to standardize the names

@nigel-sampson
Copy link
Collaborator

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
@mvegaca mvegaca added the Can Close Out Soon Work relating to this issue has been completed. label Apr 11, 2018
@mrlacey
Copy link
Collaborator Author

mrlacey commented Apr 12, 2018

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

@mrlacey mrlacey closed this as completed Apr 12, 2018
@ghost ghost locked as resolved and limited conversation to collaborators Jun 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Can Close Out Soon Work relating to this issue has been completed. enhancement Indicates the issue is a suggestion for an improvement or alteration to something that already exist fall-creators-update The issue relates to functionality released as part of the Fall Creators Update release of Windows 1
Projects
None yet
Development

No branches or pull requests

6 participants