-
Notifications
You must be signed in to change notification settings - Fork 6.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Run] Fluent UX #28538
[Run] Fluent UX #28538
Conversation
This comment has been minimized.
This comment has been minimized.
This seems really cool, +1 |
@niels9001 |
|
This comment has been minimized.
This comment has been minimized.
@niels9001 |
This looks fire 🔥 |
@niels9001 |
@niels9001 |
Yeah, those are decreased to be inline with Windows standards.. once this gets merged feel free to create an issue to add this setting.. maybe a bit too much to do that in this PR as it's already pretty major? |
Nope, seems like Settings loads these images as well.. looks better now, as they are now all consistent :)!
Great catch! Fixed! |
This comment was marked as spam.
This comment was marked as spam.
Overview should also consider the EDIT: what do you think about adding a button that opens PT Run settings in the overview? |
Nice catch. Thought they are disabled in that case.
What do you use as trigger? |
Callback on plugin changing |
Yes please, feel free to push! Yeah that's not a bad idea.. we can see where it would fit best, but would be helpful to have it somewhere |
Tested on Windows 10 and it's not crashing anymore 😃
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still LGTM! Thank you for the contribution!
And now working on Windows 10! Just have some nits.
...cher/Plugins/Community.PowerToys.Run.Plugin.VSCodeWorkspaces/VSCodeHelper/VSCodeInstances.cs
Outdated
Show resolved
Hide resolved
src/modules/launcher/Plugins/Microsoft.PowerToys.Run.Plugin.System/Images/shutdown.light-1.png
Outdated
Show resolved
Hide resolved
Does this fully cover #29627? |
Thanks! See comments
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave it a test on Win11, didn't find any issues. Nice work!
Let's wait for @jaimecbernardo to give it another test on win10 before merging.
I actually wish I could test this stuff without needing to install everything to compile myself. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks a lot for the contribution! 🚀
Summary of the Pull Request
This PR moves Run from
ModernWpf
toWpfUI
, and introduces the following changes:Open questions / to-do's:
ThemeManager
withoutModernWpf
(and remove that dependency)?Before vs. after:
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed