Skip to content

Conversation

@niels9001
Copy link
Collaborator

@niels9001 niels9001 commented Jun 24, 2025

Summary of the Pull Request

New dashboard UX

Dashboard.mp4

Update available:
image

No update available:
image

Updated KeyVisual and Shortcut control

  • Refactoring KeyVisual to remove redundant properties and UI elements, and using Styles for better customization.
  • Shortcut control now shows a "Configure shortcut" label when there's no shortcut configured.
    image

Other changes

  • Consolidated converters that were used across pages in App.xaml.cs with consistent naming.
  • Renamed templated controls (from .cs to .xaml.cs) and moving those to the Controls root folder vs. individual folders for a better overview.

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

@niels9001 niels9001 changed the title [UX] New dashboard [UX] New dashboard & refactored KeyVisual Jun 27, 2025
@niels9001 niels9001 marked this pull request as ready for review July 2, 2025 17:28
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@crutkas
Copy link
Member

crutkas commented Jul 8, 2025

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@github-actions

This comment has been minimized.

@Jaylyn-Barbee
Copy link
Contributor

There is one last a11y issue that will be fixed when this is merged and we update CommunityToolkit/Windows#702

IsOpen="True"
Severity="Informational"
Visibility="{x:Bind ViewModel.IsAnimationEnabledBySystem, Mode=OneWay, Converter={StaticResource BoolToInvertedVisibilityConverter}}">
Visibility="{x:Bind ViewModel.IsAnimationEnabledBySystem, Mode=OneWay, Converter={StaticResource BoolToReverseVisibilityConverter}}">
Copy link
Member

Choose a reason for hiding this comment

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

Good catch. Same question on review.

We had 2 issues with the refactoring of KeyVisual that happened in
#40214:

- We had little control over the pixel perfection of the Windows + text
+ glyph KeyVisual templates, which resulted into subpar vertical
alignments.
- When changing theme, the Windows key foreground brush was not
updating.

With this PR
- a new control with dedicated templates is introduced; this allows us
to have full control over the individual templates, and doing all of the
styling in XAML vs in C#.
- I've removed setting the accessible names because those should be set
on the parent container, as Narrator is supposed to announce these
shortcuts in its totality vs individual keys.
- A new property `RenderKeyAsGlyph` has been introduced to render a key
as an icon (to save space in certain scenarios) or to show the full text
(default)

<img width="133" height="147" alt="image"
src="https://github.com/user-attachments/assets/66a342da-fdcf-4c97-80b6-93b9955ebdd3"
/>

<img width="242" height="58" alt="image"
src="https://github.com/user-attachments/assets/0b41cfee-8a7d-409a-83b4-72d13e0b131c"
/>
@DHowett DHowett merged commit c91bef1 into main Aug 4, 2025
14 checks passed
@yeelam-gordon yeelam-gordon requested a review from Copilot August 5, 2025 04:28

This comment was marked as outdated.

@yeelam-gordon yeelam-gordon requested a review from Copilot August 5, 2025 05:23
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a new dashboard UX design and refactors the KeyVisual component for better customization. The changes focus on improving the user experience by replacing the old module layout with an organized dashboard featuring separate sections for quick access actions and shortcuts overview, while also updating the KeyVisual control to use a more flexible style-based approach.

  • New dashboard UX with organized sections for actions and shortcuts
  • Refactored KeyVisual component to remove redundant properties and enable style-based customization
  • Consolidated converters and updated resource references for consistency

Reviewed Changes

Copilot reviewed 36 out of 43 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/settings-ui/Settings.UI/ViewModels/DashboardViewModel.cs Refactored to support new dashboard layout with separate collections for shortcuts and actions
src/settings-ui/Settings.UI/ViewModels/DashboardModuleItem.cs Replaced complex KBM item with simpler activation item model
src/settings-ui/Settings.UI/SettingsXAML/Views/DashboardPage.xaml Complete redesign of dashboard layout with card-based sections
src/settings-ui/Settings.UI/SettingsXAML/Controls/KeyVisual/* Refactored KeyVisual control with new templating system and styles
src/settings-ui/Settings.UI/Strings/en-us/Resources.resw Updated UI strings to match new dashboard terminology

var list = new List<DashboardModuleItem>
{
new DashboardModuleButtonItem() { ButtonTitle = resourceLoader.GetString("EnvironmentVariables_LaunchButtonControl/Header"), IsButtonDescriptionVisible = true, ButtonDescription = resourceLoader.GetString("EnvironmentVariables_LaunchButtonControl/Description"), ButtonGlyph = "\uEA37", ButtonClickHandler = EnvironmentVariablesLaunchClicked },
new DashboardModuleButtonItem() { ButtonTitle = resourceLoader.GetString("EnvironmentVariables_LaunchButtonControl/Header"), IsButtonDescriptionVisible = true, ButtonDescription = resourceLoader.GetString("EnvironmentVariables_LaunchButtonControl/Description"), ButtonGlyph = "ms-appx:///Assets/Settings/Icons/EnvironmentVariables.png", ButtonClickHandler = EnvironmentVariablesLaunchClicked },
Copy link

Copilot AI Aug 5, 2025

Choose a reason for hiding this comment

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

This line is extremely long and difficult to read. Consider breaking it into multiple lines for better readability.

Suggested change
new DashboardModuleButtonItem() { ButtonTitle = resourceLoader.GetString("EnvironmentVariables_LaunchButtonControl/Header"), IsButtonDescriptionVisible = true, ButtonDescription = resourceLoader.GetString("EnvironmentVariables_LaunchButtonControl/Description"), ButtonGlyph = "ms-appx:///Assets/Settings/Icons/EnvironmentVariables.png", ButtonClickHandler = EnvironmentVariablesLaunchClicked },
new DashboardModuleButtonItem()
{
ButtonTitle = resourceLoader.GetString("EnvironmentVariables_LaunchButtonControl/Header"),
IsButtonDescriptionVisible = true,
ButtonDescription = resourceLoader.GetString("EnvironmentVariables_LaunchButtonControl/Description"),
ButtonGlyph = "ms-appx:///Assets/Settings/Icons/EnvironmentVariables.png",
ButtonClickHandler = EnvironmentVariablesLaunchClicked
},

Copilot uses AI. Check for mistakes.
Comment on lines +74 to +88
if (this != null)
{
if (IsInvalid)
{
VisualStateManager.GoToState(this, InvalidState, true);
}
else if (!IsEnabled)
{
VisualStateManager.GoToState(this, DisabledState, true);
}
else
{
VisualStateManager.GoToState(this, NormalState, true);
}
}
Copy link

Copilot AI Aug 5, 2025

Choose a reason for hiding this comment

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

The condition 'if (this != null)' is redundant since 'this' can never be null within an instance method. This check should be removed.

Suggested change
if (this != null)
{
if (IsInvalid)
{
VisualStateManager.GoToState(this, InvalidState, true);
}
else if (!IsEnabled)
{
VisualStateManager.GoToState(this, DisabledState, true);
}
else
{
VisualStateManager.GoToState(this, NormalState, true);
}
}
if (IsInvalid)
{
VisualStateManager.GoToState(this, InvalidState, true);
}
else if (!IsEnabled)
{
VisualStateManager.GoToState(this, DisabledState, true);
}
else
{
VisualStateManager.GoToState(this, NormalState, true);
}

Copilot uses AI. Check for mistakes.
}

private void Update()
{
Copy link

Copilot AI Aug 5, 2025

Choose a reason for hiding this comment

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

The Update method checks if Content is null but doesn't handle the case where _keyPresenter is null, which could cause a NullReferenceException when trying to set properties on it.

Suggested change
{
{
if (_keyPresenter == null)
{
return;
}

Copilot uses AI. Check for mistakes.

public static readonly DependencyProperty AllowDisableProperty = DependencyProperty.Register("AllowDisable", typeof(bool), typeof(ShortcutControl), new PropertyMetadata(false, OnAllowDisableChanged));

private static ResourceLoader resourceLoader = Helpers.ResourceLoaderInstance.ResourceLoader;
Copy link

Copilot AI Aug 5, 2025

Choose a reason for hiding this comment

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

Static field should be readonly since it's never reassigned after initialization. Consider making it 'private static readonly ResourceLoader resourceLoader'.

Suggested change
private static ResourceLoader resourceLoader = Helpers.ResourceLoaderInstance.ResourceLoader;
private static readonly ResourceLoader resourceLoader = Helpers.ResourceLoaderInstance.ResourceLoader;

Copilot uses AI. Check for mistakes.
{
public sealed partial class CheckUpdateControl : UserControl
{
public bool UpdateAvailable { get; set; }
Copy link

Copilot AI Aug 5, 2025

Choose a reason for hiding this comment

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

Property UpdateAvailable should be readonly since it's only set in the constructor. Consider using a private setter or making it a get-only property.

Suggested change
public bool UpdateAvailable { get; set; }
public bool UpdateAvailable { get; }

Copilot uses AI. Check for mistakes.
vanzue added a commit that referenced this pull request Aug 8, 2025
<!-- Enter a brief description/summary of your PR here. What does it
fix/what does it change/how was it tested (even manually, if necessary)?
-->
## Summary of the Pull Request
Fix a crash in settings page due to not found converter
<!-- Please review the items on the PR checklist before submitting-->

## AI Summary
This pull request makes a small update to the `MouseUtilsPage.xaml` file
to use the correct resource for converting boolean values to visibility
states in the UI.

- Updated the `Visibility` binding on an `InfoBar` to use the
`ReverseBoolToVisibilityConverter` instead of the incorrect
`BoolToReverseVisibilityConverter` resource.

## PR Checklist

- [ ] Closes: #xxx
- [ ] **Communication:** I've discussed this with core contributors
already. If the work hasn't been agreed, this work might be rejected
- [ ] **Tests:** Added/updated and all pass
- [ ] **Localization:** All end-user-facing strings can be localized
- [ ] **Dev docs:** Added/updated
- [ ] **New binaries:** Added on the required places
- [ ] [JSON for
signing](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ESRPSigning_core.json)
for new binaries
- [ ] [WXS for
installer](https://github.com/microsoft/PowerToys/blob/main/installer/PowerToysSetup/Product.wxs)
for new binaries and localization folder
- [ ] [YML for CI
pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ci/templates/build-powertoys-steps.yml)
for new test projects
- [ ] [YML for signed
pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/release.yml)
- [ ] **Documentation updated:** If checked, please file a pull request
on [our docs
repo](https://github.com/MicrosoftDocs/windows-uwp/tree/docs/hub/powertoys)
and link it here: #xxx

<!-- Provide a more detailed description of the PR, other things fixed,
or any additional comments/features here -->
## Detailed Description of the Pull Request / Additional comments
Regression caused by #40214

<!-- Describe how you validated the behavior. Add automated tests
wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
<img width="1983" height="1072" alt="image"
src="https://github.com/user-attachments/assets/13c806c3-e7af-4615-a649-6d58d8fe877b"
/>
yeelam-gordon pushed a commit that referenced this pull request Aug 8, 2025
<!-- Enter a brief description/summary of your PR here. What does it
fix/what does it change/how was it tested (even manually, if necessary)?
-->
## Summary of the Pull Request
Fixed toggle switch not working issue.

## AI Summary
This pull request refactors how `DashboardListItem` objects are created
and added to collections in the `DashboardViewModel`. The main
improvement is to separate the instantiation of each `DashboardListItem`
from the assignment of its `EnabledChangedCallback` property, which is
now set after the object is added to the relevant collection. This
change improves clarity and may help prevent issues related to object
initialization order.

Refactoring of `DashboardListItem` creation and initialization:

* In the `AddDashboardListItem` method, the `DashboardListItem` object
is now created and added to `AllModules` before its
`EnabledChangedCallback` property is set, instead of setting this
property during object initialization.
* In the `GetShortcutModules` method, both `ShortcutModules` and
`ActionModules` collections now receive `DashboardListItem` objects that
are instantiated first, added to the collection, and then have their
`EnabledChangedCallback` property set. This replaces the previous
pattern of setting the callback during object creation.
[[1]](diffhunk://#diff-aea3404667e7a3de2750bf9ab7ee8ff5e717892caa68ee1de86713cf8e21b44cL123-R136)
[[2]](diffhunk://#diff-aea3404667e7a3de2750bf9ab7ee8ff5e717892caa68ee1de86713cf8e21b44cL144-R159)
* 
<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist

- [x] Closes: #41046
- [ ] **Communication:** I've discussed this with core contributors
already. If the work hasn't been agreed, this work might be rejected
- [ ] **Tests:** Added/updated and all pass
- [ ] **Localization:** All end-user-facing strings can be localized
- [ ] **Dev docs:** Added/updated
- [ ] **New binaries:** Added on the required places
- [ ] [JSON for
signing](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ESRPSigning_core.json)
for new binaries
- [ ] [WXS for
installer](https://github.com/microsoft/PowerToys/blob/main/installer/PowerToysSetup/Product.wxs)
for new binaries and localization folder
- [ ] [YML for CI
pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ci/templates/build-powertoys-steps.yml)
for new test projects
- [ ] [YML for signed
pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/release.yml)
- [ ] **Documentation updated:** If checked, please file a pull request
on [our docs
repo](https://github.com/MicrosoftDocs/windows-uwp/tree/docs/hub/powertoys)
and link it here: #xxx

<!-- Provide a more detailed description of the PR, other things fixed,
or any additional comments/features here -->
## Detailed Description of the Pull Request / Additional comments
It is an regression from
#40214

<!-- Describe how you validated the behavior. Add automated tests
wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed

---------

Signed-off-by: Shuai Yuan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-User Interface things that regard UX for PowerToys Needs-Review This Pull Request awaits the review of a maintainer. Product-Settings The standalone PowerToys Settings application

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dashboard/Feature overview [Setting > Shortcut control] Show red info text on empty shortcut

9 participants