Updating navigation tree for settings with groupings #35559
Conversation
| xmlns="" | ||
| xmlns:xsd="http://www.w3.org/2001/XMLSchema" | ||
| xmlns:msdata="urn:schemas-microsoft-com:xml-msdata"> | ||
| <xsd:schema id="root" xmlns="" xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:msdata="urn:schemas-microsoft-com:xml-msdata"> |
There was a problem hiding this comment.
this was VS automatically adjusting this, fyi
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -111,7 +111,11 @@ private void OnItemInvoked(NavigationViewItemInvokedEventArgs args) | |||
| .OfType<NavigationViewItem>() | |||
| .First(menuItem => (string)menuItem.Content == (string)args.InvokedItem); | |||
| var pageType = item.GetValue(NavHelper.NavigateToProperty) as Type; | |||
| NavigationService.Navigate(pageType); | |||
|
|
|||
| if (pageType != null) | |||
There was a problem hiding this comment.
without the L1 having a nav target, it would crash
This comment has been minimized.
This comment has been minimized.
| <NavigationViewItem | ||
| x:Uid="Shell_Hosts" | ||
| helpers:NavHelper.NavigateTo="views:HostsPage" | ||
| Icon="{ui:BitmapIcon Source=/Assets/Settings/Icons/Hosts.png}" /> |
There was a problem hiding this comment.
Feels more like a (os) system tool. What makes the difference between advanced and system tools?
| <NavigationViewItem | ||
| x:Uid="Shell_EnvironmentVariables" | ||
| helpers:NavHelper.NavigateTo="views:EnvironmentVariablesPage" | ||
| Icon="{ui:BitmapIcon Source=/Assets/Settings/Icons/EnvironmentVariables.png}" /> |
There was a problem hiding this comment.
Feels more like a (os) system tool. What makes the difference between advanced and system tools?
| x:Uid="Shell_TopLevelSystemTools" | ||
| Icon="Page2" | ||
| SelectsOnInvoked="False"> | ||
| <NavigationViewItem.MenuItems> |
There was a problem hiding this comment.
Except of Awake and ShortcutGuide this tools feel more like advanced tools instead of (os) system tools.
|
Maybe I screwed up mentally testing this. I know I validate l2 moved but maybe i saw it when I only had top level and bottoms and I mentally checked the bottom and then thought it worked |
…y simplier and just leverages the builti in stuff as well
|
@crutkas Would you like to link this PR to the correct ISSUE? |
|
@Jay-o-Way, why isn't this ready? #23744 is the issue (great call out). We've been discussing options for months. Everyone will have opinions on groupings and goal is "best of the options" due to it. @Aaron-Junker actually came up with more or less the current list and it mirrored a few of our internal ones too. Updating docs here can be done pretty quick and benign to this change i believe. if there is something larger i'm missing, i'm all ears. i know how to do the larger iterative effort for improving left nav and @ethanfangg is on point for future improvements of oobe/scoobe + dashboard. |
|
Oh I saw some (open) comments, and words like "next steps", but I suppose those are for a later pr. I'll have to give it a better look again soon. P.S. this change is going to make many people happy 😊 |
|
This is working and looks good after adding the null check that Jeremy suggested. |
|
Certainly not the optimal solution, but it's a working one. View codediff --git a/src/settings-ui/Settings.UI/SettingsXAML/Views/ShellPage.xaml.cs b/src/settings-ui/Settings.UI/SettingsXAML/Views/ShellPage.xaml.cs
index 5e1a199044..3c9beed618 100644
--- a/src/settings-ui/Settings.UI/SettingsXAML/Views/ShellPage.xaml.cs
+++ b/src/settings-ui/Settings.UI/SettingsXAML/Views/ShellPage.xaml.cs
@@ -4,7 +4,7 @@
using System;
using System.Collections.Generic;
-
+using System.Linq;
using ManagedCommon;
using Microsoft.PowerToys.Settings.UI.Helpers;
using Microsoft.PowerToys.Settings.UI.Services;
@@ -122,6 +122,8 @@ namespace Microsoft.PowerToys.Settings.UI.Views
public static bool IsUserAnAdmin { get; set; }
+ private static Dictionary<Type, NavigationViewItem> _navViewParentLookup;
+
/// <summary>
/// Initializes a new instance of the <see cref="ShellPage"/> class.
/// Shell page constructor.
@@ -138,6 +140,21 @@ namespace Microsoft.PowerToys.Settings.UI.Views
// shellFrame.Navigate(typeof(GeneralPage));
IPCResponseHandleList.Add(ReceiveMessage);
SetTitleBar();
+
+ if (_navViewParentLookup == null)
+ {
+ _navViewParentLookup = new Dictionary<Type, NavigationViewItem>();
+
+ var topLevelItems = navigationView.MenuItems.OfType<NavigationViewItem>().ToArray();
+
+ foreach (var parent in topLevelItems)
+ {
+ foreach (var child in parent.MenuItems.OfType<NavigationViewItem>())
+ {
+ _navViewParentLookup.TryAdd(child.GetValue(NavHelper.NavigateToProperty) as Type, parent);
+ }
+ }
+ }
}
public static int SendDefaultIPCMessage(string msg)
@@ -348,6 +365,12 @@ namespace Microsoft.PowerToys.Settings.UI.Views
if (selectedItem != null)
{
Type pageType = selectedItem.GetValue(NavHelper.NavigateToProperty) as Type;
+
+ if (_navViewParentLookup.TryGetValue(pageType, out var parentItem) && !parentItem.IsExpanded)
+ {
+ parentItem.IsExpanded = true;
+ }
+
NavigationService.Navigate(pageType);
}
} |
Co-authored-by: Jeremy Sinclair <[email protected]>
|
Added @davidegiacometti suggested code, baller suggestion |
|
The last change I made was suspicious so I investigate on why it was needed. CC @snickler for your suggestion |
|
Next steps are #35621 |
Summary of the Pull Request
#23744
We've grown bigger over time, and with that, we need to make is easier to find stuff.
Next steps
Search, pinning will be next 'large' phase.
Here is current state with L1 icons
Fully expanded (but older screenshot
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed