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.

Sign up for GitHub

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

Jump to bottom

ColorPicker Updates #4502

Closed
wants to merge 8 commits into from
Closed

ColorPicker Updates #4502

wants to merge 8 commits into from

Conversation

robloo
Copy link
Contributor

@robloo robloo commented Mar 5, 2022 •
edited
Loading

Fixes / Updates

Relates to updates mentioned in #3643

  1. Create a new ColorPreviewer primitive and use it in the ColorPicker
  2. Remove obsolete ColorToColorShadeConverter (replaced by AccentColorConverter)
  3. Use a corner radius of 4 to match Fluent v2
  4. Switch to using NumberBox for color channel inputs
  5. [WIP] Add back drop shadow behind preview color

Closes #3643, #4208

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature

What is the current behavior?

What is the new behavior?

PR Checklist

Please check if your PR fulfills the following requirements:

  • Created a feature/dev branch in your fork (vs. submitting directly from a commit on main)
  • Based off latest main branch of toolkit
  • Tested code with current supported SDKs
  • New component
    • Pull Request has been submitted to the documentation repository instructions. Link:
    • Added description of major feature to project description for NuGet package (4000 total character limit, so don't push entire description over that)
    • If control, added to Visual Studio Design project
  • Sample in sample app has been added / updated (for bug fixes / features)
  • New major technical changes in the toolkit have or will be added to the [Wiki](https://github.com/CommunityToolkit/WindowsCommunityToolkit/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
    - Very minor changes to templates and removed an obsolete converter

Other information

@ghost
Copy link

ghost commented Mar 5, 2022

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 🙌

@ghost ghost requested review from michael-hawker and azchohfi March 5, 2022 18:18
@ghost ghost added bug 🐛 An unexpected issue that highlights incorrect behavior bugbash 🏗️ controls 🎛️ labels Mar 5, 2022
@robloo robloo mentioned this pull request Mar 5, 2022
Open
2 tasks
@ghost ghost added the feature request 📬 A request for new changes to improve functionality label Mar 5, 2022
@robloo
Copy link
Contributor Author

robloo commented Mar 5, 2022 •
edited
Loading

I attempted to add back a drop shadow using the new AttachedDropShadow. AttachedDropShadow is a very poor development experience compared to DropShadowPanel which nicely encapsulated related ideas into a container control. Now there are major limitations it seems and its just a thin wrapper over the way composition works (which is also poorly designed and doesn't fit in with XAML).

Concerns aside: I will need someone to recommend fixes for the drop shadow in ColorPreviewer.xaml. The code I have doesn't work and I'm not sure why.

  1. Can't use x:Bind in a template
  2. Binding by ElementName can't work in a resource dictionary
  3. AttachedDropShadow can't exist as a child with other UI elements

michael-hawker
michael-hawker reviewed Mar 7, 2022
View reviewed changes
Microsoft.Toolkit.Uwp.UI.Controls.Input/ColorPicker/ColorPreviewer.xaml
CornerRadius="4"
HorizontalAlignment="Stretch"
VerticalAlignment="Stretch" />
<Border x:Name="PreviewBorder"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a rounded rectangle element, AttachedCardShadow should just be used here. Note that you need to wrap it in another border (as in the sample) for that to work as it's a primitive. That should make this easier.

Copy link
Contributor Author

@robloo robloo Mar 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AttachedCardShadow should just be used here.

I believe that is in the Microsoft.Toolkit.Uwp.UI.Media package/namespace so I can't use it. That was what I tried first.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦‍♂️ I realized thinking that last night and forgot when I got to commenting here today. 🤔 I'll have to pull down the branch and take a look. Is the current issue that it's not displaying correctly? I have some thoughts on what may be happening. FYI @Ryken100, I know you've been poking at some alternatives too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the current issue that it's not displaying correctly?

The shadow isn't displaying on the target at all. I think it can't even find the target border due to namescope issues but am not sure.

@robloo robloo mentioned this pull request Apr 27, 2022
Merged
3 tasks
@michael-hawker michael-hawker added this to the 8.0 milestone Aug 9, 2022
@michael-hawker michael-hawker mentioned this pull request Aug 9, 2022
Closed
14 tasks
@robloo
Copy link
Contributor Author

robloo commented Aug 20, 2022

Note that this PR is awaiting feedback from the repository maintainers on what to do with drop shadows. See discussions above.

@michael-hawker michael-hawker self-assigned this Sep 19, 2023
@michael-hawker
Copy link
Member

Wanted to drop a note to say this hasn't been forgotten. ColorPicker just didn't make the cut for porting to the new repo for our 8.0 release. It's been highly missed though, so we'll want to bring it forward in 8.1. @niels9001 @Arlodotexe we should use this PR as a base for the port. @niels9001 maybe we can investigate the updates here, the new Segmented control, and how we can deal with the styling for shadow (or non-shadow style) since I know we've been doing a pass on Win 11 style updates as we port.

@niels9001 niels9001 mentioned this pull request Sep 23, 2023
Merged
@robloo
Copy link
Contributor Author

robloo commented Oct 23, 2023

While I've stepped away from UWP/WinUI3 for now please note the definitive version of this control is now in Avalonia. There were many enhancements and lessons learned applied there.

Only some of those enhancements are in this PR. If this control is going to be in 8.1 it might be a good idea to port back some of those ideas.

@robloo robloo closed this Sep 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michael-hawker michael-hawker michael-hawker left review comments

@azchohfi azchohfi Awaiting requested review from azchohfi

Assignees

@michael-hawker michael-hawker

Labels
bug 🐛 An unexpected issue that highlights incorrect behavior bugbash 🏗️ controls 🎛️ feature request 📬 A request for new changes to improve functionality
Projects
Windows Community Toolkit
Status: Done
Milestone
8.0
Development

Successfully merging this pull request may close these issues.

ColorPicker Bottom Shade Adjuster Resets Color to White [Feature] ColorPickerButton Updates
2 participants
@robloo @michael-hawker