WindowsCommunityToolkit icon indicating copy to clipboard operation
WindowsCommunityToolkit copied to clipboard

[Feature] ColorPickerButton Updates

Open robloo opened this issue 3 years ago • 20 comments

Describe the problem this feature would solve

Roll-up of all feature additions and changes needed for the next version of the ColorPickerButton.

Original PR https://github.com/windows-toolkit/WindowsCommunityToolkit/pull/3379 Original Issue https://github.com/windows-toolkit/WindowsCommunityToolkit/issues/3363

Features / Refactoring

  1. [ ] [Cancelled] ~Add a recent colors palette / list~
    • ~It may make sense to show this in place of the accent colors and the preview color (there isn't space for both). The recent color list will always show the currently selected color first anyway. At this point an enum for preview display modes may make more sense than a lot of mutually exclusive properties: 1. PreviewColor, 2. PreviewAndAccentColors 3. RecentColors~
  2. [ ] [Cancelled] ~Add an Eyedropper button to select an existing color in the app. Integrate with the recent colors list. Follow ColorPickerUX~
    • ~The recent colors will likely be horizontal instead of vertical and replacing the standard preview and accent colors~
    • ~The eyedropper will be shown to the left of the recent colors list (nearest the currently selected color)~
  3. [x] Add property to hide the accent colors to show only the preview color
  4. [ ] [Cancelled] ~Address todo items from initial PR here:~
    • ~The mini selected color palette may follow the Windows accent color lighter/darker shades algorithm. However, the implementation of this algorithm is currently unknown. This algorithm may be in the XAML fluent theme editor: https://github.com/microsoft/fluent-xaml-theme-editor~ Algorithm is unknown and likely will never be public. I'm dropping this change from consideration until/if the situation changes.
    • ~Move localizable strings into resources~
    • ~Treat negative/zero numbers in CustomPaletteColumnCount as 'auto' and automatically calculate a good column count to keep rows/columns even (square root rounded up to the nearest int). Set the default of this property to 0 'auto'.~
  5. [ ] [Cancelled] ~Complete the ColorPickerSlider as a stand-alone control. Use binding to link it with the ColorPicker/ColorPickerButton. This simplifies the template and removes the remaining unwanted template parts.~
  6. [x] Combine all property changed callbacks into one to simplify things [From discord discussion] Closed in #4134
  7. [x] Use a corner radius of 4 to match WinUI 2.6 styles
  8. [x] Switch to using NumberBox for color channel inputs
  9. [ ] Add back drop shadow behind preview color
  10. [x] Make accent colors work in HSV to avoid colors getting stuck in black/white. Will fix https://github.com/CommunityToolkit/WindowsCommunityToolkit/issues/4208.

Bugs

  1. [x] There seems to be a visual bug in slider background rendering in some edge cases. This is a result of fixing some components to max in cases they probably shouldn't be.
  2. [x] Set default 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] Closed in #4134
  3. [ ] DPI changes do not trigger redrawing checkered background or slider gradients. This causes them to be blurry. Must listen for DPI changes and then re-render the backgrounds. [From discord discussion]

If I missed anything feel free to add in the comments below!

Additional context & Screenshots

robloo avatar Dec 21 '20 04:12 robloo

Hello, 'robloo! Thanks for submitting a new feature request. I've automatically added a vote 👍 reaction to help get things started. Other community members can vote to help us prioritize this feature in the future!

ghost avatar Dec 21 '20 04:12 ghost

@robloo these will be a great addition to the ColorPickerButton which is already an amazing feature.

Some of our team members are out for holidays but I will open this up for discussion with the rest of the community to get further input.

Kyaa-dost avatar Dec 21 '20 19:12 Kyaa-dost

Thanks @Kyaa-dost, I do plan to work on most of these myself when the time comes. I'm just started to collect everything together for discussion and so nothing is forgotten.

robloo avatar Dec 21 '20 20:12 robloo

Thanks, @robloo! Looking forward to test these features when ready.

Kyaa-dost avatar Dec 21 '20 21:12 Kyaa-dost

Thanks @robloo for creating a tracking for this. Is there a split of some polish items you'd like to get in 7.0 that's closing down within the first part of January? Or are you thinking these would all come later for a 7.1?

Just a note, we have the Eyedropper control in the Toolkit already you can leverage, but it uses Win2D currently, so that would move the ColorPicker to that package if we do. See #3594 for more details on our work on reducing dependencies.

michael-hawker avatar Dec 22 '20 07:12 michael-hawker

@michael-hawker No plans to make any changes for 7.0. I was thinking 7.1 timeframe.

That said, if an issue is found I will get a fix out ASAP and might take a look at one or two other things at the same time. Really just waiting on wider feedback before more changes too.

Just a note, we have the Eyedropper control in the Toolkit already you can leverage, but it uses Win2D currently, so that would move the ColorPicker to that package if we do.

Ah, did not know the restructuring of packages was that in-depth. Thanks for the link! I don't have any concerns if the ColorPicker has to move to the Win2D package if that's what you want. I expect to use the existing Eyedropper.

robloo avatar Dec 22 '20 22:12 robloo

Ah, did not know the restructuring of packages was that in-depth. Thanks for the link! I don't have any concerns if the ColorPicker has to move to the Win2D package if that's what you want. I expect to use the existing Eyedropper.

Good to know. We may want to put the ColorPicker in that package now then so we don't move it for 7.1, though then that raises the dependencies required to pull it into an app, so not sure if that matters to you. Though anyone who pulls the Microsoft.Toolkit.Uwp.UI.Controls package would still get it by default as it'll include everything in the Toolkit.

It's just if for some reason a developer didn't want to pull in Win2D, they could exclude it if they wanted by grabbing smaller packages.

Thoughts?

michael-hawker avatar Jan 06 '21 21:01 michael-hawker

@robloo was just thinking/wondering if we provide a content area there and have some code in the Media package which knows how to hook-in an EyeDropperButton if desired...

We'll at least be pulling in the WinUI dependency so we can update on top of the latest ColorPicker, but there's not much that gains us from not making the new ColorPicker super heavy if it's going to require Win2D by default.

michael-hawker avatar Feb 05 '21 03:02 michael-hawker

@michael-hawker It's your call where the ColorPickerButton should go in terms of packaging. This is one of the cases where creating too many separate packages can cause issues as a control may not clearly fit into one of the rigid groups.

ColorPickerButton should be available in the standard controls I think. In the future it should also have eyedropper functionality. The two seem to be at odds with one another.

robloo avatar Feb 05 '21 15:02 robloo

@robloo yeah, that's certainly a concern about having very specific packages. The main thing with the EyeDropper is just the Win2D dependency is a heavy thing not everyone is going to want to add for it, though for apps that would want a ColorPicker with an EyeDropper they'd probably not mind.

I've been trying to think if there's a way we could extend the functionality of ColorPicker with the EyeDropper without a hard link between the two somehow... @azchohfi @nmetulev thoughts on this topic/problem?

michael-hawker avatar Feb 05 '21 17:02 michael-hawker

@robloo are you planning to make more improvements to the ColorPicker/ColorPickerButton in the next month or so for our 7.1 release? #4010 should be merged today (didn't notice the auto-merge got stuck the other day).

michael-hawker avatar Jun 29 '21 17:06 michael-hawker

@michael-hawker I certainly won't have time to get to the full list above. However, I may try to finish 1 or 2. I would say the chances I will have time are low but we should still keep this on 7.1 for now.

robloo avatar Jun 29 '21 17:06 robloo

@robloo I know some of these were fixed in #4134, are they all done as part of that or are there some remaining? Should we split off what's left into a new issue or leave this one for tracking?

michael-hawker avatar Aug 31 '21 16:08 michael-hawker

@michael-hawker This one is basically an epic for tracking. I always keep it up to date and I've already included the changes in #4134 (the boxes checked so far are for that PR).

robloo avatar Aug 31 '21 16:08 robloo

Thanks @robloo issues are used to generate release notes; so it could be good to have a separate one with the things fixed in 7.1 that we can close and have rolled into that list too.

For now moved your main tracking one here to 7.2/8.0 release.

michael-hawker avatar Aug 31 '21 18:08 michael-hawker

Can we tag this as epic? Epics are handled differently and should be ignored by automated release notes in most cases. I won't have the time to manage a bunch of separate issues for the ColorPicker.

That said, the main fix in the PR already had a separate issue #4121. As mentioned in the PR, this issue is only related.

robloo avatar Aug 31 '21 19:08 robloo

In the next update I plan to tackle all features/issues except eyedropper and recent colors palette. Assuming the DropShadow replacement API is ready that will be included too.

Target for completing this will be September.

robloo avatar Sep 01 '21 00:09 robloo

Thanks @robloo yeah, DropShadow API is shipping with 7.1 that we're closing out this week, so it'll be there for you in September. The main thing with the EyeDropper is that control we have in the Toolkit currently is part of our Win2D Media package, so that'd be a hard dependency to add, so we may have to think about how that interaction works. Beyond that though, the composition based drop shadow is in the UI package, so it should be available within the Input package.

michael-hawker avatar Sep 01 '21 16:09 michael-hawker

@michael-hawker

Update considering the following:

  • The official deprecation of UWP
  • WinUI 3 not being ready (missing features (several Microsoft doesn't even list) and bugs)
  • Uno Platform not yet supporting WCT on WinUI3
  • General fuzziness around Microsoft-led .NET/XAML tech at the moment (cancelled meetings, pushed schedules, .NET SDK community issues)

No time is going to be invested here for a bit. I expect I will get around to these updates in a few months as the internal version of this control is probably going to get several of these updates. I'll then copy that code over here.

Off topic: Frankly, long-term I expect to move to Avalonia and Uno Platform exclusively due to Microsoft's history of XAML decision making (nothing against you and the WCT toolkit, just something to pass along to your management if it comes up).

robloo avatar Oct 26 '21 18:10 robloo

@robloo we've appreciated your contributions to the Toolkit, and I appreciate you giving us feedback here. If anything changes, you're always welcome to contribute here in the future still of course.

michael-hawker avatar Oct 27 '21 21:10 michael-hawker