-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was VS automatically adjusting this, fyi
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except of Awake and ShortcutGuide this tools feel more like advanced tools instead of (os) system tools.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have pushed a small change since the proposed code wasn't working for the second time after collapsing an expanded parent.
I have tested the navigation from the navigation view, dashboard, OOBE and utility like Hosts and ColorPicker.
Ship it! 🚀
The last change I made was suspicious so I investigate on why it was needed. CC @snickler for your suggestion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Withstood every scenario I've tested. Great work!
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