CommunityToolkit / WindowsCommunityToolkit

The Windows Community Toolkit is a collection of helpers, extensions, and custom controls. It simplifies and demonstrates common developer tasks building .NET apps with UWP and the Windows App SDK / WinUI 3 for Windows 10 and Windows 11. The toolkit is part of the .NET Foundation.
https://docs.microsoft.com/windows/communitytoolkit/
Other
5.88k stars 1.38k forks source link

[Feature] ColorPickerButton Updates #3643

Open robloo opened 3 years ago

robloo commented 3 years ago

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

ghost commented 3 years ago

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!

Kyaa-dost commented 3 years ago

@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.

robloo commented 3 years ago

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.

Kyaa-dost commented 3 years ago

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

michael-hawker commented 3 years ago

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.

robloo commented 3 years ago

@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.

michael-hawker commented 3 years ago

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 commented 3 years ago

@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.

robloo commented 3 years ago

@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.

michael-hawker commented 3 years ago

@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 commented 3 years ago

@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).

robloo commented 3 years ago

@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.

michael-hawker commented 3 years ago

@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?

robloo commented 3 years ago

@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).

michael-hawker commented 3 years ago

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.

robloo commented 3 years ago

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 commented 3 years ago

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.

michael-hawker commented 3 years ago

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.

robloo commented 2 years ago

@michael-hawker

Update considering the following:

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).

michael-hawker commented 2 years ago

@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.