Skip to content

Refactor SUI navigation#19889

Merged
carlos-zamora merged 8 commits intomainfrom
dev/cazamor/sui/refactor-navigation
Feb 25, 2026
Merged

Refactor SUI navigation#19889
carlos-zamora merged 8 commits intomainfrom
dev/cazamor/sui/refactor-navigation

Conversation

@carlos-zamora
Copy link
Member

Summary of the Pull Request

Consolidates the navigation functions in MainPage for the settings UI. This involved:

  • combining all separate _Navigate() functions into one big one
  • deduplicating SettingsNav().SelectedItem() calls
  • removing the unnecessary variable for a crumb (just inline creation and registration)
  • removing the elementToFocus staging behavior for color schemes and profile sub pages

References and Relevant Issues

Builds off the work done in #19519 and #19831

Validation Steps Performed

Navigate to...
✅ simple top-level pages: Startup, Interaction, Appearance, Rendering, Compatibility, Add profile
✅ Color schemes and subpages
✅ Actions, subpages
✅ New Tab Menu and folder subpages
✅ Extensions, subpages, and profile/scheme navigation
✅ defaults profile and subpages
✅ specific profile and subpages

Also tested discarding changes on these pages.

✅ search still works and navigates to the correct element

PR Checklist

Closes #19866

Base automatically changed from dev/cazamor/sui/search to main February 20, 2026 23:04
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high water mark R456

SettingsNav().SelectedItem(firstItem);
_Navigate(firstItem.Tag(), BreadcrumbSubPage::None);

_UpdateSearchIndex();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(updating the index post-navigation?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the one in UpdateSettings(). We discussed offline that we do still need this, but we both forgot why haha. That said, the whole point of the build time index is that it's not supposed to change.

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Feb 20, 2026
@carlos-zamora carlos-zamora force-pushed the dev/cazamor/sui/refactor-navigation branch from 7496435 to f2f5ec9 Compare February 20, 2026 23:29
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Feb 20, 2026
@carlos-zamora
Copy link
Member Author

^ that force-push was a git rebase --onto main 5da5177f2. No merge conflicts and tested it locally (albeit briefly). Diff should be much cleaner now.

// Method Description:
// - Navigates to the page corresponding to the given nav tag. Updates the breadcrumb bar and selected nav view item accordingly.
// Arguments:
// - vm: the nav tag of the page to navigate to. Can be either an hstring for static pages or

Check failure

Code scanning / check-spelling

Forbidden Pattern Error

an matches a line_forbidden.patterns entry: "\\san (?=(?:[b-df-gj-np-rtv-xz]|h(?!our|sl|tml|ttp)|s(?!sh|vg))[a-z])". (forbidden-pattern)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm guessing it means "an hstring" is illegal

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this bothers me more than it should haha

@github-actions

This comment was marked as resolved.

{
// Used to trigger the PropertyChanged handler in MainPage.cpp
// This forces the page to refresh
_NotifyChanges(L"CurrentPage");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huh, what was this for/where did it go?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_Navigate() now supports navigating to subpages directly, so we have no need for the force refresh functionality anymore.

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LFG. Does this fix the issue where you can't navigate into Edit Action more than once?

@carlos-zamora
Copy link
Member Author

LFG. Does this fix the issue where you can't navigate into Edit Action more than once?

Yes! That sounds familiar!

@carlos-zamora carlos-zamora enabled auto-merge (squash) February 25, 2026 21:27
@carlos-zamora carlos-zamora merged commit b4259e6 into main Feb 25, 2026
18 of 20 checks passed
@carlos-zamora carlos-zamora deleted the dev/cazamor/sui/refactor-navigation branch February 25, 2026 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consolidate SUI navigation functions

2 participants