Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Run] Fluent UX #28538

Merged
merged 34 commits into from Nov 20, 2023
Merged

[Run] Fluent UX #28538

merged 34 commits into from Nov 20, 2023
+770 −743

Conversation

niels9001
Copy link
Contributor

@niels9001 niels9001 commented Sep 13, 2023 •
edited

Summary of the Pull Request

This PR moves Run from ModernWpf to WpfUI, and introduces the following changes:

Open questions / to-do's:

  • Can we use ThemeManager without ModernWpf (and remove that dependency)?
  • VS Workspaces plugin dark/light icon support
  • This needs proper testing on Windows 10 vs. 11.

RunVNext2

Before vs. after:
Frame 1010108473

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

@niels9001 niels9001 added Status-In progress This issue or work-item is under development Area-User Interface things that regard UX for PowerToys Product-PowerToys Run Improved app launch PT Run (Win+R) Window labels Sep 13, 2023
@github-actions

This comment has been minimized.

Sign in to view
@tiagorangel2011
Copy link

This seems really cool, +1

@htcfreek
Copy link
Collaborator

@niels9001
Can you fix the wrong PowerToys plugin's icon in Settings too?

@niels9001
Copy link
Contributor Author

@niels9001 Can you fix the wrong PowerToys plugin's icon in Settings too?

Do you mean this one?
image

@github-actions

This comment has been minimized.

Sign in to view
@htcfreek
Copy link
Collaborator

htcfreek commented Sep 13, 2023 •
edited

@niels9001 Can you fix the wrong PowerToys plugin's icon in Settings too?

Do you mean this one?
image

@niels9001
yes. But for settings we need a font based version. Or not?

@Vucko130
Copy link

This looks fire 🔥

@htcfreek
Copy link
Collaborator

@niels9001
Did you changed the font size? For me it was easier to read with the old one. Can you make it configurable (e.g. drop-down: big or normal)?

@htcfreek
Copy link
Collaborator

@niels9001
The name of the uri icon for dark mode has a different name. It should be uri.dark.png to follow convention.

@htcfreek htcfreek mentioned this pull request Sep 14, 2023
Closed
@niels9001
Copy link
Contributor Author

@niels9001 Did you changed the font size? For me it was easier to read with the old one. Can you make it configurable (e.g. drop-down: big or normal)?

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?

@niels9001
Copy link
Contributor Author

@niels9001 Can you fix the wrong PowerToys plugin's icon in Settings too?

Do you mean this one?
image

@niels9001 yes. But for settings we need a font based version. Or not?

Nope, seems like Settings loads these images as well.. looks better now, as they are now all consistent :)!

@niels9001 The name of the uri icon for dark mode has a different name. It should be uri.dark.png to follow convention.

Great catch! Fixed!

@github-actions

This comment was marked as spam.

Sign in to view
@davidegiacometti
Copy link
Collaborator

davidegiacometti commented Sep 15, 2023 •
edited

Overview should also consider the IsPluginInitialized property.
I managed to make the overview refreshing when plugins or action keyword changes. Can I push or PR to this branch?

EDIT: what do you think about adding a button that opens PT Run settings in the overview?

@htcfreek
Copy link
Collaborator

Overview should also consider the IsPluginInitialized property.

Nice catch. Thought they are disabled in that case.

I managed to make the overview refreshing when plugins or action keyword changes.

What do you use as trigger?

@davidegiacometti
Copy link
Collaborator

davidegiacometti commented Sep 15, 2023 •
edited by niels9001

Overview should also consider the IsPluginInitialized property. I managed to make the overview refreshing when plugins or action keyword changes. Can I push or PR to this branch?

EDIT: what do you think about adding a button that opens PT Run settings in the overview?

Callback on plugin changing Disabled or ActionKeyword property.

@niels9001
Copy link
Contributor Author

niels9001 commented Sep 15, 2023 •
edited

Overview should also consider the IsPluginInitialized property. I managed to make the overview refreshing when plugins or action keyword changes. Can I push or PR to this branch?

EDIT: what do you think about adding a button that opens PT Run settings in the overview?

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

@crutkas crutkas added the Needs-Review This Pull Request awaits the review of a maintainer. label Nov 15, 2023
@crutkas crutkas marked this pull request as ready for review November 15, 2023 17:14
@davidegiacometti
Copy link
Collaborator

Tested on Windows 10 and it's not crashing anymore 😃
Pushed a commit with:

  • A missing font family due to new utility merged (verified that the font family is needed for Windows 10)
  • Reintroduced callback to refresh overview (missed during alignment with main)

@niels9001 niels9001 changed the title 🚧 [Run] Fluent UX [Run] Fluent UX Nov 16, 2023
jaimecbernardo
jaimecbernardo requested changes Nov 16, 2023
View reviewed changes
Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a 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
@Jay-o-Way
Copy link
Collaborator

Does this fully cover #29627?

@niels9001
Copy link
Contributor Author

Had two questions ⬆️

Thanks! See comments

Does this fully cover #29627?
The PowerOCR PR got merged yesterday that includes the fixes for that specific module. This PR will address the Segoe MDL2 icons for Run. Maybe in the future we should create separate issues to better track/close these, as PRs going across multiple modules is a bit of a hassle. I've added the In progress tag for now, but we should be able to close it once this gets merged 😊!

stefansjfw
stefansjfw approved these changes Nov 17, 2023
View reviewed changes
Copy link
Collaborator

@stefansjfw stefansjfw left a 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.

@Theoyeah
Copy link

I actually wish I could test this stuff without needing to install everything to compile myself.

jaimecbernardo
jaimecbernardo approved these changes Nov 20, 2023
View reviewed changes
Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a 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! 🚀

@jaimecbernardo jaimecbernardo merged commit c095cdd into main Nov 20, 2023
14 checks passed
@jaimecbernardo jaimecbernardo removed Status-In progress This issue or work-item is under development Needs-Review This Pull Request awaits the review of a maintainer. labels Nov 20, 2023
@crutkas crutkas deleted the niels9001/run-fluent-run branch November 22, 2023 06:08
@lukaszjablonski lukaszjablonski mentioned this pull request Dec 5, 2023
Open
@niels9001 niels9001 mentioned this pull request Dec 9, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@htcfreek htcfreek htcfreek left review comments

@Jay-o-Way Jay-o-Way Jay-o-Way requested changes

@jaimecbernardo jaimecbernardo jaimecbernardo approved these changes

@stefansjfw stefansjfw stefansjfw approved these changes

Assignees
No one assigned
Labels
Area-User Interface things that regard UX for PowerToys Product-PowerToys Run Improved app launch PT Run (Win+R) Window
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet