-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
[Settings/ImageResizer] New UI for sizes listview #12269
[Settings/ImageResizer] New UI for sizes listview #12269
Conversation
@niels9001 |
Should we add a close button? Is the users asked before deletion? |
What about Cancel? |
I am sure the values will be saved directly. So a cancel button makes no sense. |
src/settings-ui/Microsoft.PowerToys.Settings.UI/Views/ImageResizerPage.xaml
Outdated
Show resolved
Hide resolved
src/settings-ui/Microsoft.PowerToys.Settings.UI/Views/ImageResizerPage.xaml
Outdated
Show resolved
Hide resolved
src/settings-ui/Microsoft.PowerToys.Settings.UI/Views/ImageResizerPage.xaml
Outdated
Show resolved
Hide resolved
src/settings-ui/Microsoft.PowerToys.Settings.UI/Strings/en-us/Resources.resw
Outdated
Show resolved
Hide resolved
What about the Add size button: does it use the same fly-out? Asking because of #8698. |
@htcfreek @Jay-o-Way a cancel/close button isn't needed for a Flyout since it's light dismissible. I have added a delete confirmation dialog. |
Will every body know how to close it? 🤔
👍 |
What about this, @niels9001 ? |
I think we should have a default name for a new size to be added.. that's a separate PR IMO and touches core imageresize classes. |
@check-spelling-bot ReportUnrecognized words, please review:
To accept these unrecognized words as correct, run the following commands... in a clone of the [email protected]:microsoft/PowerToys.git repository
If you see a bunch of garbageIf it relates to a ... well-formed patternSee if there's a pattern that would match it. If not, try writing one and adding it to the Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines. Note that patterns can't match multiline strings. binary-ish stringPlease add a file path to the File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.
|
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.
src/settings-ui/Microsoft.PowerToys.Settings.UI/Views/ImageResizerPage.xaml
Show resolved
Hide resolved
Fixed the spellcheck value and the missing dialog button labels |
@niels9001 |
@niels9001 |
nit: Should delete prompt by closable on Esc? |
#7043 |
@stefansjfw @jaimecbernardo I have replaced the MessageDialog with a ContentDialog. The dialog is now rendered correctly, is closeable with ESC and now includes the image size name in the title. This should address all outstanding feedback :). |
src/settings-ui/Microsoft.PowerToys.Settings.UI/Views/ImageResizerPage.xaml
Outdated
Show resolved
Hide resolved
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.
Needs a little fix.
The rest looks good to me.
Great work!
src/settings-ui/Microsoft.PowerToys.Settings.UI/Strings/en-us/Resources.resw
Outdated
Show resolved
Hide resolved
Sooner or later, yes. See #8698 |
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!
Great work!
Summary of the Pull Request
Problems resolved:
The new application features text-based UI and a dedicated edit button that provides a fly-out that enables the user to make changes to the selected image size.
Ideally, we'd move to a solution as described here: #2813 (comment). However, due to a blocking XAML Island bug (a ContentDialog does not accept keyboard entry: microsoft/microsoft-ui-xaml#3804) we can not implement that.
Quality Checklist
Contributor License Agreement (CLA)
A CLA must be signed. If not, go over here and sign the CLA.