Skip to content

Refactor SUI navigation#19889

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

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

Conversation

@carlos-zamora

Copy link
Copy Markdown
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
Comment thread src/cascadia/TerminalSettingsEditor/MainPage.cpp

@DHowett DHowett left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

high water mark R456

Comment thread src/cascadia/TerminalSettingsEditor/MainPage.cpp Outdated
Comment thread src/cascadia/TerminalSettingsEditor/MainPage.cpp Outdated
SettingsNav().SelectedItem(firstItem);
_Navigate(firstItem.Tag(), BreadcrumbSubPage::None);

_UpdateSearchIndex();

Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread src/cascadia/TerminalSettingsEditor/MainPage.cpp
Comment thread src/cascadia/TerminalSettingsEditor/MainPage.cpp Outdated
@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

@DHowett DHowett left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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

3 participants