Color picker fixes #4134

robloo · 2021-07-27T02:51:16Z

~~👉 It is imperative to resolve ONE ISSUE PER PR and avoid making multiple changes unless the changes interrelate with each other~~

Fixes #4121

Relates to #3643

[Bug] Ensures that the selected 'panel' or 'tab' is visible at first load. This fixes Color Picker custom style not load on selected pane #4121.
[Bug] Set default ColorPickerSlider DefaultForeground property Brush values in the XAML default style instead of when registering the DP. This is necessary to ensure a reference isn't shared across instances or threads. [From discord discussion]
Make tabs automatically size uniformly to width. This improves the design when some tabs are hidden
Add central OnDependencyPropertyChanged method in ColorPicker. This simplifies some code and makes adding future property changed handling easier.

PR Type

What kind of change does this PR introduce?

Bugfix
Feature

Refactoring (no functional changes, no api changes)

What is the current behavior?

What is the new behavior?

PR Checklist

Please check if your PR fulfills the following requirements:

Tested code with current supported SDKs
Pull Request has been submitted to the documentation repository instructions. Link:
Sample in sample app has been added / updated (for bug fixes / features)
- Icon has been created (if new sample) following the Thumbnail Style Guide and templates
New major technical changes in the toolkit have or will be added to the Wiki e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
Tests for the changes have been added (for bug fixes / features) (if applicable)
Header has been added to all new source files (run build/UpdateHeaders.bat)
Contains NO breaking changes

Other information

ghost · 2021-07-27T02:51:19Z

Thanks robloo for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

robloo · 2021-07-27T02:55:27Z

Microsoft.Toolkit.Uwp.UI.Controls.Input/ColorPicker/ColorPicker.cs

@@ -216,6 +220,7 @@ protected override void OnApplyTemplate()

            base.OnApplyTemplate();
            this.UpdateVisualState(false);
+            this.ValidateSelectedPanel();


There is an issue with validating and updating the selected panel only here. If the IsColorSpectrumVisible or IsColorChannelTextInputVisible properties are changed after the template is applied the selected tab may become invalid. This is because the base WinUI ColorPicker is currenlty responsible for updating the visual state that actually applies tab visibility for these two (it is all done in the style). Only IsColorPaletteVisible is managed by the derived toolkit ColorPicker. To fix this:

Need to monitor for property changes on these properties as well, or

Need to connect to VisualState changed events -- there are more than one and this is a bit ugly.

This is an existing issue, correct?

Yes, I was waiting to see if its considered enough of an issue to fix. The changes now will work as long as the properties are updated in XAML which is by far the largest use case.

robloo · 2021-07-27T02:58:25Z

Overall the selection fix exposes an inconvenience with separating a selection control (ListBox) from the presenter content control (SwitchPresenter). Ideally, the control responsible for selection could be smart enough to ensure the selected item is actually visible. It is a bit more complex to do this in the control itself and obviously isn't general purpose. A nice 'Tab' control to handle this case would be better in my opinion.

This improves the design when some tabs are hidden

This fixes potential issues with shared instances of the brush

Microsoft.Toolkit.Uwp.UI.Controls.Input/ColorPicker/ColorPickerSlider.xaml

Rosuavio

It seems the new example is covering an existing one.

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/ColorPicker/ColorPickerButtonXaml.bind

michael-hawker · 2021-08-03T16:10:30Z

@robloo couple of open questions and a small fix to the sample when you have a chance, thanks!

Waiting on smaple changes

robloo · 2021-08-18T02:19:02Z

@michael-hawker Looks like there are a lot more changes to the template than I initially thought. You really moved a lot of things around when you broke up ColorPicker and ColorPickerButton. One of the things you removed appears to be the drop shadow behind the primary preview color (the one in the center). I'm planning to add this drop shadow back unless you had a good reason for taking it out. The comments look like you just forgot to add it back.

WindowsCommunityToolkit/Microsoft.Toolkit.Uwp.UI.Controls.Input/ColorPicker/ColorPicker.xaml

Lines 453 to 461 in 5171b99

    
                           <!--<controls:DropShadowPanel Grid.RowSpan="3" 
        
                                                     Grid.Column="2" 
        
                                                     HorizontalContentAlignment="Stretch" 
        
                                                     VerticalContentAlignment="Stretch" 
        
                                                     BlurRadius="10.0" 
        
                                                     OffsetX="0.0" 
        
                                                     OffsetY="0.0" 
        
                                                     ShadowOpacity="0.75" 
        
                                                     Color="Black"> Not too far off without it, maybe we can fake?-->

Otherwise, I should get to finishing the PR next week.

michael-hawker · 2021-08-19T22:53:39Z

@robloo can't remember why I removed it, think there was some rendering issue I was fighting. FYI, we're going to deprecate the DropShadowPanel as we're adding new shadows to the toolkit in #4179. Maybe just leave it still for now and we can resolve later for the next release?

robloo · 2021-08-21T18:07:21Z

@michael-hawker Sure, I'll wait until the new drop shadow methodology is in place. It's good there is advancement in this area! I also read you are updating to 2.6 styles so I can include the drop shadow with some teaks to make things look better with WinUI 2.6 styles. (at least some 2px -> 4px corner radius updates)

michael-hawker · 2021-08-25T20:40:14Z

@robloo everything else about this PR was ready to go, eh? We can add a fix in for the DropShadow as a separate PR once #4179 is merged, eh?

robloo · 2021-08-25T20:52:47Z

@michael-hawker No, not ready to go. Need to fix the samples. I still plan on getting to that this week.

This is now the same as ColorPicker samples

ghost · 2021-08-29T17:05:12Z

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

robloo · 2021-08-29T17:13:40Z

This should be all set now

Rosuavio

Looks good!

ghost · 2021-08-30T20:11:59Z

Hello @michael-hawker!

Because this pull request has the auto merge :zap: label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

michael-hawker · 2021-08-30T21:12:23Z

Thanks @robloo for continued improvements to the Color Picker!

robloo added 2 commits July 26, 2021 22:50

Ensure only visible tabs are selected

90e8a34

Add additional color picker samples showing only a palette

f448ee3

ghost requested review from michael-hawker, azchohfi and Rosuavio July 27, 2021 02:51

ghost added bug 🐛 An unexpected issue that highlights incorrect behavior controls 🎛️ labels Jul 27, 2021

robloo commented Jul 27, 2021

View reviewed changes

robloo added 3 commits July 26, 2021 23:30

Make tabs automatically size uniformly to width

bc05c86

This improves the design when some tabs are hidden

Set slider DefaultForeground value in style

e23a895

This fixes potential issues with shared instances of the brush

Add central OnDependencyPropertyChanged method in ColorPicker

dde93f3

michael-hawker added the next preview ✈️ Label for marking what we want to include in the next preview release for developers to try. label Jul 27, 2021

michael-hawker added this to the 7.1 milestone Jul 27, 2021

michael-hawker assigned Rosuavio and XAML-Knight Jul 29, 2021

XAML-Knight approved these changes Jul 29, 2021

View reviewed changes

Microsoft.Toolkit.Uwp.UI.Controls.Input/ColorPicker/ColorPickerSlider.xaml Show resolved Hide resolved

Rosuavio suggested changes Aug 2, 2021

View reviewed changes

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/ColorPicker/ColorPickerButtonXaml.bind Outdated Show resolved Hide resolved

ghost added the needs author feedback 📝 label Aug 2, 2021

michael-hawker removed the next preview ✈️ Label for marking what we want to include in the next preview release for developers to try. label Aug 3, 2021

Rosuavio previously approved these changes Aug 4, 2021

View reviewed changes

Fix ColorPickerButton samples and switch to StackPanel

79f2931

This is now the same as ColorPicker samples

ghost added needs attention 👋 and removed needs author feedback 📝 labels Aug 29, 2021

Merge branch 'master' into color-picker-fixes

9f20cf4

Rosuavio approved these changes Aug 30, 2021

View reviewed changes

michael-hawker added auto merge ⚡ and removed needs attention 👋 labels Aug 30, 2021

Merge branch 'main' into color-picker-fixes

4fd1609

michael-hawker added the next preview ✈️ Label for marking what we want to include in the next preview release for developers to try. label Aug 30, 2021

michael-hawker merged commit aecbcad into CommunityToolkit:main Aug 30, 2021

robloo deleted the color-picker-fixes branch August 30, 2021 22:06

michael-hawker mentioned this pull request Aug 31, 2021

[Feature] ColorPickerButton Updates #3643

Open

13 tasks

robloo mentioned this pull request Sep 7, 2021

ColorPicker not working unoplatform/uno#6834

Open

1 task

Rosuavio removed their assignment Sep 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Color picker fixes #4134

Color picker fixes #4134

robloo commented Jul 27, 2021 •

edited

Loading

ghost commented Jul 27, 2021

robloo Jul 27, 2021 •

edited

Loading

Rosuavio Aug 3, 2021

robloo Aug 3, 2021

robloo commented Jul 27, 2021

Rosuavio left a comment •

edited

Loading

michael-hawker commented Aug 3, 2021

robloo commented Aug 18, 2021

michael-hawker commented Aug 19, 2021

robloo commented Aug 21, 2021

michael-hawker commented Aug 25, 2021

robloo commented Aug 25, 2021

ghost commented Aug 29, 2021

robloo commented Aug 29, 2021

Rosuavio left a comment

ghost commented Aug 30, 2021

michael-hawker commented Aug 30, 2021

Color picker fixes #4134

Color picker fixes #4134

Conversation

robloo commented Jul 27, 2021 • edited Loading

Fixes #4121

PR Type

What is the current behavior?

What is the new behavior?

PR Checklist

Other information

ghost commented Jul 27, 2021

robloo Jul 27, 2021 • edited Loading

Choose a reason for hiding this comment

Rosuavio Aug 3, 2021

Choose a reason for hiding this comment

robloo Aug 3, 2021

Choose a reason for hiding this comment

robloo commented Jul 27, 2021

Rosuavio left a comment • edited Loading

Choose a reason for hiding this comment

michael-hawker commented Aug 3, 2021

robloo commented Aug 18, 2021

michael-hawker commented Aug 19, 2021

robloo commented Aug 21, 2021

michael-hawker commented Aug 25, 2021

robloo commented Aug 25, 2021

ghost commented Aug 29, 2021

robloo commented Aug 29, 2021

Rosuavio left a comment

Choose a reason for hiding this comment

ghost commented Aug 30, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

michael-hawker commented Aug 30, 2021

robloo commented Jul 27, 2021 •

edited

Loading

robloo Jul 27, 2021 •

edited

Loading

Rosuavio left a comment •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.