Color picker wpf #37149
Color picker wpf #37149
Conversation
| <StackPanel | ||
| HorizontalAlignment="Center" | ||
| VerticalAlignment="Center" | ||
| AutomationProperties.LiveSetting="Assertive" | ||
| AutomationProperties.Name="{x:Static p:Resources.Copied_to_clipboard}" | ||
| Orientation="Horizontal"> | ||
| <ui:SymbolIcon | ||
| <TextBlock | ||
| FontFamily="Segoe Fluent Icons, Segoe MDL2 Assets" |
There was a problem hiding this comment.
Tip: never hardcode values like this. This is super sensitive to typos and other reasons for failure. Use a FontIcon instead :)
There was a problem hiding this comment.
There is no FontIcon in .net WPF.
There was a problem hiding this comment.
Can we make this a resource and link to it? I can imagine we have these at various places?
e.g.
FontFamily="{StaticResource IconFontFamily}"
There was a problem hiding this comment.
I've made the change.
| x:Name="OKButton" | ||
| Grid.Row="9" | ||
| Grid.ColumnSpan="4" | ||
| Margin="0,32,0,0" | ||
| HorizontalAlignment="Stretch" | ||
| Appearance="Primary" | ||
| Background="{DynamicResource {x:Static SystemColors.AccentColorBrushKey}}" |
There was a problem hiding this comment.
Is it possible to use {StaticResource} with a value from WinUI Gallery?
There was a problem hiding this comment.
We don't use WinUI but .net WPF.
src/modules/colorPicker/ColorPickerUI/Controls/ColorPickerControl.xaml
Outdated
Show resolved
Hide resolved
src/modules/colorPicker/ColorPickerUI/Controls/ColorFormatControl.xaml
Outdated
Show resolved
Hide resolved
|
First image on the left shows a button with black text on a green background. This is not good accessibility. Let's see how to fix that. (my general advice: stick with the basics) |
src/modules/colorPicker/ColorPickerUI/Views/ColorEditorView.xaml
Outdated
Show resolved
Hide resolved
|
The code looks good, but I do have one small concern: the original UI didn’t seem to have any problems, so why switch to .NET WPF? |
|
I've updated the select button to use the accent style. Below are the screenshots for W11. |
src/modules/colorPicker/ColorPickerUI/Controls/ColorPickerControl.xaml
Outdated
Show resolved
Hide resolved
| @@ -274,7 +292,8 @@ | |||
| TextWrapping="Wrap" /> | |||
| </StackPanel> | |||
|
|
|||
| <Border | |||
| <Border | |||
| Background="{DynamicResource {x:Static SystemColors.AccentColorBrushKey}}" | |||
There was a problem hiding this comment.
| Background="{DynamicResource {x:Static SystemColors.AccentColorBrushKey}}" | |
| BorderBrush="{DynamicResource SurfaceStrokeColorFlyoutBrush}" |
There was a problem hiding this comment.
Do you mean to replace BorderBrush to SurfaceStrokeColorFlyoutBrush and to remove the Background? Or just replace the BorderBrush?
…rol.xaml Co-authored-by: Niels Laute <[email protected]>
crutkas
added
the
Needs-Review
There was a problem hiding this comment.
Copilot reviewed 4 out of 12 changed files in this pull request and generated 1 comment.
Files not reviewed (8)
- src/modules/colorPicker/ColorPickerUI/App.xaml: Language not supported
- src/modules/colorPicker/ColorPickerUI/ColorEditorWindow.xaml: Language not supported
- src/modules/colorPicker/ColorPickerUI/ColorPickerUI.csproj: Language not supported
- src/modules/colorPicker/ColorPickerUI/Controls/ColorFormatControl.xaml: Language not supported
- src/modules/colorPicker/ColorPickerUI/Controls/ColorPickerControl.xaml: Language not supported
- src/modules/colorPicker/ColorPickerUI/Resources/Styles.xaml: Language not supported
- src/modules/colorPicker/ColorPickerUI/Views/ColorEditorView.xaml: Language not supported
- src/modules/colorPicker/ColorPickerUI/Views/ZoomView.xaml: Language not supported
Comments suppressed due to low confidence (1)
src/modules/colorPicker/ColorPickerUI/Controls/ColorPickerControl.xaml.cs:385
- [nitpick] The method name
GetValueFromNumberBoxis outdated as it now usesTextBoxinstead ofNumberBox. Consider renaming it toGetValueFromTextBox.
private static byte GetValueFromNumberBox(TextBox numberBox, byte previousValue)
| @@ -357,15 +356,19 @@ private void HexCode_GotKeyboardFocus(object sender, KeyboardFocusChangedEventAr | |||
| (sender as System.Windows.Controls.TextBox).SelectAll(); | |||
| } | |||
|
|
|||
| private void TextBox_PreviewTextInput(object sender, TextCompositionEventArgs e) | |||
| { | |||
| e.Handled = !System.Text.RegularExpressions.Regex.IsMatch(e.Text, "^[0-9]+$"); | |||
There was a problem hiding this comment.
The regex should allow hexadecimal characters (0-9, a-f, A-F) for color codes. Update the regex to: "^[0-9a-fA-F]+$".
| e.Handled = !System.Text.RegularExpressions.Regex.IsMatch(e.Text, "^[0-9]+$"); | |
| e.Handled = !System.Text.RegularExpressions.Regex.IsMatch(e.Text, "^[0-9a-fA-F]+$"); |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
Summary of the Pull Request
Use .net WPF instead of WPFUI.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Comparison Windows 11. Left side new one, right side old one.
Light
Dark
Aquatic
Desert
Dusk
Night Sky
Validation Steps Performed
Test on W11 and W10, dark and light and all HC themes.