Skip to content

Add New Tab Menu Customization to Settings UI#18015

Merged
carlos-zamora merged 24 commits intomainfrom
dev/cazamor/SUI/newTabMenu
Dec 3, 2024
Merged

Add New Tab Menu Customization to Settings UI#18015
carlos-zamora merged 24 commits intomainfrom
dev/cazamor/SUI/newTabMenu

Conversation

@carlos-zamora
Copy link
Member

@carlos-zamora carlos-zamora commented Oct 8, 2024 •
edited
Loading

Summary of the Pull Request

Adds customization for the New Tab Menu to the settings UI.

  • Settings Model changes:
    • The Settings UI generally works by creating a copy of the entire settings model objects on which we apply the changes to. Turns out, we completely left the NewTabMenu out of that process. So I went ahead and implemented it.
    • FolderEntry
      • FolderEntry exposes Entries() (used by the new tab menu to figure out what to actually render) and RawEntries() (the actual JSON data deserialized into settings model objects). I went ahead and exposed RawEntries() since we'll need to apply changes to it to then serialize.
  • View Model:
    • NewTabMenuViewModel is the main view model that interacts with the page. It maintains the current view of items and applies changes to the settings model.
    • NewTabMenuEntryViewModel and all of the other _EntryViewModel classes are wrappers for the settings model NTM entries.
    • FolderTreeViewEntry encapsulates FolderEntryViewModel. It allows us to construct a TreeView of just folders.
  • View changes and additions:
    • Added FontIconGlyph to the SettingContainer
    • Added a New Tab Menu item to the navigation view
    • Adding entries: a stack of SettingContainers is used here. We use the new FontIconGlyph to make this look nice!
    • Reordering entries: drag and drop is supported! This might not work in admin mode though, and we can't drag and drop into folders. Buttons were added to make this keyboard accessible.
    • To move entries into a folder, a button was added which then displays a TreeView of all folders.
    • Multiple entries can be moved to a folder or deleted at once!
    • Breadcrumbs are used for folders
    • When a folder is entered, additional controls are displayed to customize that folder.
       

Verification

  • ✅ a11y pass
  • ✅ keyboard accessible
  • scenarios:
    • ✅ add entries (except actions)
    • ✅ changes propagated to settings model (aka "saving works")
    • ✅ reorder entries
    • ✅ move entries to an existing folder
    • ✅ delete multiple entries
    • ✅ delete individual entries
    • ✅ display entries (including actions)

Follow-ups

  • add support for adding and editing action entries
  • when we discard changes or save, it would be cool if we could stay on the same page
  • allow customizing the folder entry before adding it (current workaround is to add it, then edit it)
  • improve UI for setting icon (reuse UI from Improve Profile.Icon UI in settings #17965)

github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 8, 2024
View reviewed changes
@carlos-zamora
Copy link
Member Author

carlos-zamora commented Oct 8, 2024

Demo

Top-level view

{8394C645-A9CD-4DCD-95D2-013E355B40A6}

"Move to folder" dialog

{3B76D1D9-8505-4B17-B376-ED5A5133CCB6}

Edit Folder view

{07AB9F54-A387-4E01-8B9C-C3A5FD5F36C1}

@github-actions

This comment has been minimized.

Sign in to view
@carlos-zamora carlos-zamora marked this pull request as ready for review October 9, 2024 22:44
lhecker
lhecker reviewed Oct 11, 2024
View reviewed changes
Copy link
Member

@lhecker lhecker left a comment

Choose a reason for hiding this comment

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

TBH nothing in this PR was a problem for me, although there are some nits. I'm mostly curious how it'll feel in practice (once it's in Canary). 🙂

@carlos-zamora

This comment was marked as resolved.

Sign in to view
PankajBhojwani
PankajBhojwani requested changes Nov 15, 2024
View reviewed changes
Copy link
Contributor

@PankajBhojwani PankajBhojwani left a comment

Choose a reason for hiding this comment

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

Played around with the branch, it's super cool! Mostly just nits but a couple blockers particularly around the repeated revoking/setting up of the same event handler

@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 Nov 15, 2024
@carlos-zamora
Copy link
Member Author

carlos-zamora commented Nov 19, 2024 •
edited
Loading

Feedback from Bug Bash (11/19)

  • Should "Delete Selected Items" not be enabled if there are no items selected?
  • ^ Same with "Move selected entries to folder..."
  • "delete item button" different height from reorder buttons
  • Make ListView proportionally larger (WinUI makes it hard to drag & drop sometimes, and instead scrolls)
  • When a new entry is added into ListView, we should scroll to it
  • Pill in NavViewItem disappears when "Discard changes" from nested folder view
  • Crashes when I add my PowerShell profile that has no icon into a nested folder
  • Moving a folder into itself deletes the folder
    • Create a folder entry
    • Select it
    • Click "move selected entries to folder"
    • Select itself to move into
    • Folder gets deleted
    • FIXED:
      • TreeView doesn't have a SelectionChanged event 😭. So we aren't able to update IsPrimaryButtonEnabled smartly. Instead, we'll just allow the user to proceed forward and silently fail. If multiple items were selected, move all of them except for the selected/destination folder.
  • I tried to move nothing (no selection!) into and it crashed
  • actually moving anything into root from root broke

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Attention The core contributors need to come back around and look at this ASAP. and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Nov 19, 2024
github-advanced-security[bot]
github-advanced-security bot found potential problems Nov 21, 2024
View reviewed changes
@github-actions

This comment has been minimized.

Sign in to view
github-advanced-security[bot]
github-advanced-security bot found potential problems Nov 22, 2024
View reviewed changes
@github-actions

This comment has been minimized.

Sign in to view
PankajBhojwani
PankajBhojwani approved these changes Nov 22, 2024
View reviewed changes
Copy link
Contributor

@PankajBhojwani PankajBhojwani left a comment

Choose a reason for hiding this comment

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

Looks and feels great! Excited for this

@carlos-zamora
Copy link
Member Author

carlos-zamora commented Nov 26, 2024

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Nov 26, 2024

Azure Pipelines successfully started running 1 pipeline(s).

@carlos-zamora carlos-zamora mentioned this pull request Dec 3, 2024
Open
6 tasks
@carlos-zamora carlos-zamora merged commit 0d846ae into main Dec 3, 2024
@carlos-zamora carlos-zamora deleted the dev/cazamor/SUI/newTabMenu branch December 3, 2024 23:07
@carlos-zamora carlos-zamora mentioned this pull request Feb 19, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lhecker lhecker lhecker approved these changes

@PankajBhojwani PankajBhojwani PankajBhojwani approved these changes

Assignees

No one assigned

Labels

Needs-Attention The core contributors need to come back around and look at this ASAP.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@carlos-zamora @lhecker @PankajBhojwani