Skip to content
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

[Feature] ColorPickerButton Updates #3643

Open
7 of 13 tasks
robloo opened this issue Dec 21, 2020 · 20 comments · May be fixed by #4502
Open
7 of 13 tasks

[Feature] ColorPickerButton Updates #3643

robloo opened this issue Dec 21, 2020 · 20 comments · May be fixed by #4502
Labels
feature request 📬 A request for new changes to improve functionality In-PR 🚀 open discussion ☎️

Comments

@robloo
Copy link
Contributor

robloo commented Dec 21, 2020

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 #3379
Original Issue #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. 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. Combine all property changed callbacks into one to simplify things [From discord discussion] Closed in Color picker fixes #4134
  7. Use a corner radius of 4 to match WinUI 2.6 styles
  8. Switch to using NumberBox for color channel inputs
  9. Add back drop shadow behind preview color
  10. Make accent colors work in HSV to avoid colors getting stuck in black/white. Will fix ColorPicker Bottom Shade Adjuster Resets Color to White #4208.

Bugs

  1. 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. 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 Color picker fixes #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 robloo added the feature request 📬 A request for new changes to improve functionality label Dec 21, 2020
@ghost
Copy link

ghost commented Dec 21, 2020

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
Copy link
Contributor

@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
Copy link
Contributor Author

robloo commented Dec 21, 2020

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
Copy link
Contributor

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

@michael-hawker
Copy link
Member

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
Copy link
Contributor Author

robloo commented Dec 22, 2020

@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 michael-hawker added this to the 7.1 milestone Jan 6, 2021
@michael-hawker michael-hawker added this to To do in Features 7.1 via automation Jan 6, 2021
@michael-hawker
Copy link
Member

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
Copy link
Member

@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
Copy link
Contributor Author

robloo commented Feb 5, 2021

@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
Copy link
Member

@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
Copy link
Member

@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
Copy link
Contributor Author

robloo commented Jun 29, 2021

@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 robloo mentioned this issue Jul 27, 2021
8 tasks
@michael-hawker
Copy link
Member

@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
Copy link
Contributor Author

robloo commented Aug 31, 2021

@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 michael-hawker modified the milestones: 7.1, 7.2/8.0? Aug 31, 2021
@michael-hawker michael-hawker removed this from To do in Features 7.1 Aug 31, 2021
@ghost ghost removed this from the 7.2/8.0? milestone Aug 31, 2021
@michael-hawker
Copy link
Member

michael-hawker commented Aug 31, 2021

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
Copy link
Contributor Author

robloo commented Aug 31, 2021

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
Copy link
Contributor Author

robloo commented Sep 1, 2021

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
Copy link
Member

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
Copy link
Contributor Author

robloo commented Oct 26, 2021

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

@michael-hawker
Copy link
Member

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

@robloo robloo linked a pull request Mar 5, 2022 that will close this issue
13 tasks
@ghost ghost added the In-PR 🚀 label Mar 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request 📬 A request for new changes to improve functionality In-PR 🚀 open discussion ☎️
Projects
Features 8.0
Awaiting triage
Development

Successfully merging a pull request may close this issue.

3 participants