Skip to content

Conversation

@ChenYiLins
Copy link
Contributor

Code quality: Streamlined navigation

Describe

  • Streamlined NavigationService and replaced ItemInvoked with a more concise SelectionChanged.
  • Delete ViewModel for MainWindow, put NavigationView logic into NavigationService.
  • Remove NavigationHelper, use tag in MainWindow.
  • Change Apps to System Areas.

Supplement

The reason for choosing to keep PageService is that it provides the mapping from ViewModel to Page, where page types can be managed centrally and pages can be added/removed dynamically (if needed later).

@ChenYiLins ChenYiLins added the area-winui3 Issues for winui3, version 11 or later label Apr 23, 2025
@ChenYiLins ChenYiLins added this to the Version 11 milestone Apr 23, 2025
@ChenYiLins ChenYiLins requested review from a team, Armin2208 and Spiritreader as code owners April 23, 2025 02:24
Copy link
Contributor

@Jay-o-Way Jay-o-Way left a comment

Choose a reason for hiding this comment

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

Thanks for starting on this! Suggestion: If you've started working on an issue, give us a hint, so we don't do the same issue at the same time, but independently :)

As for the PR: few comments :)

@Jay-o-Way Jay-o-Way removed this from Version 11.0 Apr 23, 2025
intellisense
@ChenYiLins
Copy link
Contributor Author

Code quality: Added updates for SelectedItem and Header

Describe

Now even if you call GoBack and other operations in the MainWindow, it will not affect the update of Header and SelectedItem.

@ChenYiLins
Copy link
Contributor Author

Code quality: Simplified Tag

Describe

Tag becomes extremely simple. φ(* ̄0 ̄)

@ChenYiLins
Copy link
Contributor Author

Code quality: Perfect keyboard and mouse operation

Describe

  • Merge keyboard and mouse operations into the Service, and the MainWindow is now clean.ヾ(≧▽≦*)o
  • Now we can use not only Alt + ←, but also Alt + →.
  • Fix the stuck problem when the App is closed in debugging mode.
  • Hide unexpected ToolTip.

Screenshots

image

@ChenYiLins
Copy link
Contributor Author

Code quality: Add subtree of items

Describe

  • Add subtree of items for Personalization item.
  • Replace FontIcon to CommunityToolkit's FontIcon.

Screenshots

image

@Jay-o-Way
Copy link
Contributor

Jay-o-Way commented Apr 24, 2025

Interesting choice to add the sub-items. Now there are two paths that do the exact same thing. You can click in the Nav, or you can click on the page. This is redundant. Need to know what @Armin2208 and @Spiritreader think about this.

@Jay-o-Way

This comment has been minimized.

@ChenYiLins
Copy link
Contributor Author

Bug

This happened when I clicked on the Theme item in the menu. Note that the SettingsCard on the page has no Click event linked to it (yet) but #947 will solve that.

Yep! This PR is based on #947, so after the merger of #947, these functions can work normally.

@ChenYiLins
Copy link
Contributor Author

Interesting choice to add the sub-items. Now there are two paths that do the exact same thing. You can click in the Nav, or you can click on the page. This is redundant. Need to know what @Armin2208 and @Spiritreader think about this.

I made this change to make the navigation structure more intuitive and clear. The reason for keeping personalized items is that users can see the descriptions of different items on this page.

@Jay-o-Way
Copy link
Contributor

Interesting choice to add the sub-items. Now there are two paths that do the exact same thing. You can click in the Nav, or you can click on the page. This is redundant. Need to know what @Armin2208 and @Spiritreader think about this.

I made this change to make the navigation structure more intuitive and clear. The reason for keeping personalized items is that users can see the descriptions of different items on this page.

Yes, but - at the moment - each Nav Item is functionally identical to the clickable Setting Cards. That's just totally unnatural and unneeded. I suggest to choose one solution.

@ChenYiLins
Copy link
Contributor Author

Yes, but - at the moment - each Nav Item is functionally identical to the clickable Setting Cards. That's just totally unnatural and unneeded. I suggest to choose one solution.

Uh... It's just like the way WinUI Gallery uses it.

@Spiritreader
Copy link
Member

Spiritreader commented Apr 26, 2025

My general recommendation (and most likely @Armin2208's) is to keep navigation functionality as close to Windows defaults as possible.

In case Windows does something that is unintuitive, Armin mostly has a pretty good radar on what to keep, so I'll get in contact with him to ask for further clarification

Also, there's the question for the personalization page in peraticular.
If theme mode is enabled, it should no longer be possible to open the non-theme mode pages and vice versa. The nav bar must also reflect this.

@Armin2208
Copy link
Member

Armin2208 commented Apr 26, 2025

Firstly, great work from you guys! The screenshots look promising, and they make me happy, considering we're finally processing to an Auto Dark Mode 11 with a new and modern User Interface.

Also, thanks for having attention to the details. Things like Alt + ← & Alt + → are really improving the user experience in the long term. You are doing a great job overall.

As a user, I would expect the following keyboard shortcut logic:

  • Alt + Left = going back to the previous page (like a webbrowser)
  • Alt + Right = going forward, to the previous page (like a webbrowser)
  • Alt + Up = going one page in navigation up
  • Alt + Down = going one page in navigation down.
  • Ctrl + Tab = going one page in navigation down.
  • Ctrl + Shift + Tab = going one page in navigation up.

From the video, I can't really tell what which button does in the current implementation. I also don't know what "XButtonUp" is.

Now speaking of the subtree menus on the navigation tree:
They should not be included. Here is why:

  • The user have either the choice of using a theme or let Auto Dark Mode manage background / mouse / accent. We need to show the user that he has a choice. The navigation tree removes this sense of choice.
  • If a theme is chosen, there is no way to set background / mouse / accent anymore. We gray them out, with a message, that they are disabled due to the use of a Windows theme. This also can't be reflected by the navigation tree.

Due to these two important factors, I recommend to not implement the navigation tree in the navigation bar. Nice idea, but sadly not a good choice from a UX perspective. But still, thanks for trying out new stuff and experimenting.

Btw: I don't know if you already have implemented the red message box in the Page Personalization, that the three entries background / mouse / accent are disabled, due to the use of a Windows theme. Maybe we can extend this red message box with a new button, which disables Windows theme for the user - because it could be that the user doesn't know how to disable it. A little idea to extend the easiness of usage.

Thanks! Looking forward to seeing more work of you guys. You can always contact me regarding UX thoughts.

@ChenYiLins
Copy link
Contributor Author

Now speaking of the subtree menus on the navigation tree: They should not be included. Here is why:

  • The user have either the choice of using a theme or let Auto Dark Mode manage background / mouse / accent. We need to show the user that he has a choice. The navigation tree removes this sense of choice.
  • If a theme is chosen, there is no way to set background / mouse / accent anymore. We gray them out, with a message, that they are disabled due to the use of a Windows theme. This also can't be reflected by the navigation tree.

Due to these two important factors, I recommend to not implement the navigation tree in the navigation bar. Nice idea, but sadly not a good choice from a UX perspective. But still, thanks for trying out new stuff and experimenting.

This is a very important point, and the existence of subtree in navigation tree will cause serious ambiguity.

Also, there's the question for the personalization page in peraticular.
If theme mode is enabled, it should no longer be possible to open the non-theme mode pages and vice versa. The nav bar must also reflect this.

Btw: I don't know if you already have implemented the red message box in the Page Personalization, that the three entries background / mouse / accent are disabled, due to the use of a Windows theme. Maybe we can extend this red message box with a new button, which disables Windows theme for the user - because it could be that the user doesn't know how to disable it. A little idea to extend the easiness of usage.

Ah ah ah ah, thank everyone very much for reminding me. I forgot this from beginning to end.(;´д`)ゞ

@ChenYiLins
Copy link
Contributor Author

Btw: I don't know if you already have implemented the red message box in the Page Personalization, that the three entries background / mouse / accent are disabled, due to the use of a Windows theme. Maybe we can extend this red message box with a new button, which disables Windows theme for the user - because it could be that the user doesn't know how to disable it. A little idea to extend the easiness of usage.

Add it in #947 now.

@ChenYiLins ChenYiLins changed the title Code quality: Streamlined navigation Code quality: Streamlined navigation and more improvements Apr 28, 2025
@Jay-o-Way
Copy link
Contributor

I like where this is going 🙂 At what point do we want to pull this into the winui branch?

@ChenYiLins
Copy link
Contributor Author

Code quality: Format xaml

Description

Using blank line separation is more conducive to quickly scanning the file structure. Symmetry can quickly identify the nesting range of tags, especially in complex XAML, which is easier to maintain.

Code quality: Operation health

Description

  • Replace bool variable with enum. At the same time, add converter in order to convert enum into Visibility.
  • Change [ObservableProperty] to adapt to Aot.
  • Changing the error output to ViewModel, the Page text may cause an error prompt that is easily misunderstood.
  • Update StateUpdateHandler, now has the function of debounce. At the same time, the events that are cleared when the watcher is started, so it is not necessary to manually clear when leaving the navigation. At the same time, some codes are improved, and now it is more secure and reliable.

@ChenYiLins
Copy link
Contributor Author

I like where this is going 🙂 At what point do we want to pull this into the winui branch?

No time like the present φ(* ̄0 ̄)

@ChenYiLins
Copy link
Contributor Author

As a user, I would expect the following keyboard shortcut logic:

  • Alt + Left = going back to the previous page (like a webbrowser)
  • Alt + Right = going forward, to the previous page (like a webbrowser)
  • Alt + Up = going one page in navigation up
  • Alt + Down = going one page in navigation down.
  • Ctrl + Tab = going one page in navigation down.
  • Ctrl + Shift + Tab = going one page in navigation up.

From the video, I can't really tell what which button does in the current implementation. I also don't know what "XButtonUp" is.

In fact, WinUI itself provides enough keyboard support, and the “Tab” key plays an important role in it, just like the setting of Win11! We may not need to add too many shortcut keys? At the same time, I forgot to mention that "XButtonUp" and "XButtonDown" are the side keys of the mouse, just like Logitech's GPW.

@ChenYiLins
Copy link
Contributor Author

Test: Let the time follow the system settings

Description

Let the time-related components of ADMApp follow the date format of the system.

@Jay-o-Way
Copy link
Contributor

Let the time follow the system settings

Works! Great job! Any change to the system format setting requires a re-load of the app/viewmodel, am I right?
image

@Jay-o-Way Jay-o-Way linked an issue Apr 29, 2025 that may be closed by this pull request
Copy link
Contributor

@Jay-o-Way Jay-o-Way left a comment

Choose a reason for hiding this comment

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

🚀

@ChenYiLins
Copy link
Contributor Author

Works! Great job! Any change to the system format setting requires a re-load of the app/viewmodel, am I right? !

Yep. I don’t think it’s necessary to constantly check if the system’s time format has changed. Honestly, I doubt any user would be so bored as to tweak the time format every other second ( pretty sure about that ( •̀ ω •́ )✧).

@Jay-o-Way Jay-o-Way merged commit 94bbf0e into AutoDarkMode:winui3 Apr 29, 2025
@ChenYiLins ChenYiLins deleted the winui3-navigation-bar branch April 29, 2025 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-winui3 Issues for winui3, version 11 or later

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Review NavMenuItems strings Rewrite Navigation logic following MS Learn examples Feature: Date format follows system settings

4 participants