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

[Settings] Plugin additional options: Allow settings description #17108

Merged
merged 6 commits into from
Mar 21, 2022
Merged

[Settings] Plugin additional options: Allow settings description #17108

merged 6 commits into from
Mar 21, 2022

Conversation

htcfreek
Copy link
Collaborator

@htcfreek htcfreek commented Mar 17, 2022 •
edited
Loading

Summary of the Pull Request

What is this about:
Currently we can't add descriptions for the additional plugin settings, We only can add the in brackets after the title. This doesn't looks good.

This PR adds the option to add descriptions for the settings.

Before After
img image

What is included in the PR:

  • Update the settings ui code.
  • Update the PluginAdditionalOptions code.
  • Update the settings of WindowWalker plugin.
  • Update the settings of TimeDate plugin. => Requires merge of [PT Run] TimeDate plugin #16662

How does someone test / validate:

Quality Checklist

Contributor License Agreement (CLA)

A CLA must be signed. If not, go over here and sign the CLA.

@htcfreek htcfreek requested a review from niels9001 March 17, 2022 16:57
@htcfreek htcfreek added Product-Settings The standalone PowerToys Settings application Run-Plugin Things that relate with PowerToys Run's plugin interface Run-Plugin Manager Issues with the PowerToys Run plugin manager Product-PowerToys Run Improved app launch PT Run (Win+R) Window labels Mar 17, 2022
@htcfreek
Copy link
Collaborator Author

htcfreek commented Mar 17, 2022 •
edited
Loading

@niels9001
Can you please help me with the margins? You should be able to merge against my branch.

@Jay-o-Way
Copy link
Collaborator

I see the text itself can be improved too

@htcfreek
Copy link
Collaborator Author

I see the text itself can be improved too

If the PR is ready you can fill reviews. Please wait until it's ready.

@niels9001
Copy link
Contributor

@htcfreek Ah, I see what you mean.. this seems to be a (visual) bug in the CheckBoxWithDescriptionControl. Could you create a issue for that so we can fix that separately??

@htcfreek
Copy link
Collaborator Author

htcfreek commented Mar 17, 2022 •
edited
Loading

Yes. I can do this. But we have to wait with merging until the settings of TimeDate plugin are updated.

cc: @jaimecbernardo

htcfreek
htcfreek commented Mar 17, 2022
View reviewed changes
src/settings-ui/Settings.UI/Controls/CheckBoxWithDescriptionControl.cs
// Add text box only if the description is not empty. Required for additional plugin options.
if (!string.IsNullOrWhiteSpace(Description))
{
panel.Children.Add(new TextBlock() { Margin = new Thickness(0, 10, 0, 0), Text = Header });
Copy link
Collaborator Author

@htcfreek htcfreek Mar 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@niels9001
FYI: If I set the second margin value here to zero, the space above the checkbox is gone as it should be. But then the header and the description are misaligned if both are present.

Maybe we have to set fixed position for the checkbox element that it isn't vertical aligned or vertical aligned withe the header.

Copy link
Collaborator Author

@htcfreek htcfreek Mar 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

New issue #17110.

@htcfreek htcfreek marked this pull request as ready for review March 17, 2022 20:21
@htcfreek htcfreek mentioned this pull request Mar 17, 2022
Closed
1 task
@htcfreek
Copy link
Collaborator Author

@htcfreek Ah, I see what you mean.. this seems to be a (visual) bug in the CheckBoxWithDescriptionControl. Could you create a issue for that so we can fix that separately??

@niels9001
Issue created: #17110

@htcfreek htcfreek self-assigned this Mar 17, 2022
@htcfreek htcfreek added the Needs-Review This Pull Request awaits the review of a maintainer. label Mar 17, 2022
@htcfreek
Copy link
Collaborator Author

@niels9001
If you are okay with the code changes, you can merge tomorrow without asking me. ;-)

@htcfreek
Copy link
Collaborator Author

I added only a description to note that the property is optional.

@htcfreek htcfreek mentioned this pull request Mar 19, 2022
Merged
11 tasks
@htcfreek
Copy link
Collaborator Author

@jaimecbernardo
Do you think we can go with the margin bug? Then this can be merged after an approval of @niels9001 . 🚀

niels9001
niels9001 approved these changes Mar 21, 2022
View reviewed changes
Copy link
Contributor

@niels9001 niels9001 left a comment

Choose a reason for hiding this comment

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

LGTM! Let's tackle the margin bug in a seperate PR!

@htcfreek
Copy link
Collaborator Author

LGTM! Let's tackle the margin bug in a seperate PR!

See the issue you assigned on

@htcfreek
Copy link
Collaborator Author

htcfreek commented Mar 21, 2022 •
edited
Loading

@jaimecbernardo
Should we merge and update the new setting in system plugin later or do we wait. I can do the change this evening

jaimecbernardo
jaimecbernardo approved these changes Mar 21, 2022
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! Thank you for the contribution!

@jaimecbernardo jaimecbernardo merged commit 380add8 into microsoft:main Mar 21, 2022
@htcfreek htcfreek deleted the PT_Settings branch March 21, 2022 20:15
@htcfreek htcfreek mentioned this pull request Mar 21, 2022
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@niels9001 niels9001 niels9001 approved these changes

@jaimecbernardo jaimecbernardo jaimecbernardo approved these changes

@Jay-o-Way Jay-o-Way Awaiting requested review from Jay-o-Way

Assignees

@htcfreek htcfreek

Labels
Needs-Review This Pull Request awaits the review of a maintainer. Product-PowerToys Run Improved app launch PT Run (Win+R) Window Product-Settings The standalone PowerToys Settings application Run-Plugin Manager Issues with the PowerToys Run plugin manager Run-Plugin Things that relate with PowerToys Run's plugin interface
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@htcfreek @Jay-o-Way @niels9001 @jaimecbernardo