Make ColorPicker button content customizable. #13073
Conversation
|
You can test this PR using the following package version. |
|
@rabbitism I think the idea here is fine. However, we need to go about implementing it differently. Its unnecessary (and overcomplicated) to add a new pseudoclass for empty and then dynamically swap out content (I also don't think this is really done in any other controls). Here is how I think it should be done:
If anyone wants to customize it for their application or usage they just set the ColorPicker.Content from a style. No need for pseudoclasses or more complicated style switching logic. If they want to keep the preview rectangle there they will need to include it in their custom style. Also keep in mind from a conceptual standpoint the DropDownButton content should not have separate parts. The ColorPicker appears exactly the same as a DropDownButton. So we don't want an extra content presenter there in addition to what the ColorPicker sets. There should only ever be ONE content set just like the DropDownButton. |
|
@robloo Yeah I though about that before, and that was my first trial, but I feel it is too surprising when someone set a content and suddently the entire color preview block is gone. I am okay to implement in that way. |
Maybe but they are customizing the control and need to know what they are doing. We should follow the theoretically best/cleanest design here and Content should just be pass-through to the DropDownButton. An alternative was done for the windows community toolkit I believe. The content was just shown on top of the color preview rectangle. The color preview rectangle was always in the background and hard-coded in the control template to be there. Not a fan of this approach either. Edit: And I suppose there would be complaints where it wasn't possible to customize the preview color rectangle -- to make it round or something -- if we left it in the template and just showed the content to the side. Passing through the content entirely to the DropDownButton allows FULL customization of the preview color rectangle as well as anything else you would like to add. |
|
You can test this PR using the following package version. |
| ContentControl.VerticalContentAlignmentProperty.AddOwner<ColorPicker>(); | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets any content displayed in the ColorPicker's DropdownButton when flyout is closed. |
There was a problem hiding this comment.
I was trying to also remove mention of the "DropdownButton" from comments like this and in the other properties.
It is an implementation detail and is not required. Some may choose to implement the ColorPicker without using a DropdownButton. I don't think it should be mentioned by name and should be more generic.
I'll keep thinking the best wording to use but something closer to "Gets or sets any content that should always be displayed whether or not the ColorView is visible."
There was a problem hiding this comment.
Your comment really makes me doubt the existence of ColorPicker itself...
There was a problem hiding this comment.
Your comment really makes me doubt the existence of ColorPicker itself...
Why do you say that? The ColorView is a "panel/canvas" type control (analogous to a Calendar). It's far too large to show in most places and you want a version of the control hidden in a flyout showing only the selection when collapsed (analygous to a CalendarDatePicker). There are no new concepts here and you can think of it this way:
- ColorPicker <-> CalendarDatePicker : A "picker" which shows only the selected value only but can expand to the full view (in a flyout/dropdown) to make a new selection.
- ColorView <-> Calendar (CalendardView in UWP) : A "panel/canvas" type control that shows the full editing view and allows making a new selection. This isn't a good fit for form-entry applications and UI that has constrained space.
As an example of what I'm talking about for the ColorPicker, see the below image. This is a version of the ColorPicker done in only a Button. There is no drop-down chevron and no DropDownButton at all. This is a perfectly valid use case and makes sense.
For these reasons I really do think we need to remove all mention of the "DropdownButton" from the comments. For most of the properties just copy/pasting what was done for ContentControl is probably enough. The ColorPicker.Content property needs a bit more detail though.
If you don't want to do this now that's fine. I'll keep thinking about it and revise at some point in the future.
There was a problem hiding this comment.
Edit: Nevermind on that last part, I see you already made the comments generic. Thanks!
There was a problem hiding this comment.
@robloo Originally color picker and color view are almost identical so I think it can be achieved by re-templating color view. But now color picker has its own property, so it's necessary to be a standalone control.
There was a problem hiding this comment.
@rabbitism Yea, that is certainly debatable. DropDownButton is actually the same situation. This is something that was done in WinUI and carried over here:
- It helps usability -- you get an actual control type
- It allows extensibility. What you did in ColorPicker here is a good example. The scaffolding was already there to modify the control. This wouldn't be possible with just a style or control theme.
…ault value to simple theme.
|
You can test this PR using the following package version. |
|
@rabbitism Thanks for adding this! Everything looks great to me :) |
|
You can test this PR using the following package version. |
Port of upstream AvaloniaUI/Avalonia#13073
What does the pull request do?
Make ColorPicker button content customizable besides the color preview rectangle.
cc @robloo
What is the current behavior?
Dropdown Button can only show selected color.
What is the updated/expected behavior with this PR?
User can use Content property to show extra info in DropdownButton.
How was the solution implemented (if it's not obvious)?
Checklist
Breaking changes
No breaking change. If Content is left null, it looks exactly the same as before this change.
Obsoletions / Deprecations
Fixed issues