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

Add new ColorView & ColorPicker Controls #8215

Merged
merged 69 commits into from
Jul 15, 2022

Conversation

robloo
Copy link
Contributor

@robloo robloo commented May 28, 2022

What does the pull request do?

Adds the last two ColorView and ColorPicker controls (in the Avalonia.Controls namespace). These are stand-alone controls not based on WinUI code. This closes #4179.

These are the final fourth and fifth major controls towards implementing the ColorPicker.

What is the current behavior?

Only the following primitives are implemented for color-related controls:

  1. ColorSpectrum spectrum #5743
  2. ColorSlider
  3. ColorPresenter

What is the updated/expected behavior with this PR?

Adds the two remaining color-related controls completing the ColorPicker functionality.

ColorView

This is the 'canvas' version of the control that does not have a drop down button. ColorPicker hosts this in a drop down button flyout.

Spectrum Palette Components
image image image

ColorPicker

This is the main control to use for color selection and editing and appears as a drop down button.

Video.mp4

How was the solution implemented (if it's not obvious)?

This is a brand-new implementation and attempts to fix past wrongs with all past color pickers starting with the name.

  • The WinUI ColorPicker isn't a picker at all (compared to other pickers) so this implementation clearly separates the ColorView from the actual ColorPicker implemented in a drop-down.
  • This version of the control attempts to do everything it can in the XAML control template keeping code-behind to an absolute minimum. This significantly increases composability and enables app developers to customize every part of these controls (and even the primitives in most cases).
  • Primitives such as the ColorSlider and ColorSpectrum can still be used separately enabling app developers to create separate color picker implementations.
  • HsvColor is heavily used in all controls and was also implemented into base Avalonia alongside Color and HslColor. This simplified code-behind and also made binding of color between primitives and controls a lot more straightforward.
  • HsvColor along with ColorSlider together unlock a lot of power with this control compared to the one in WinUI (and enable it to be easily re-templated).
  • Many new properties (more than in WinUI) were added to control all aspects of the ColorView visibility. Each tab can be separately hidden along with most individual subsections. This allows a lot of design customization without having to re-template or drill into styles.
  • New properties such as SelectedIndex and ColorModel allow customizing the control putting it in a pre-defined state. For example: the WinUI ColorPicker always defaults to RGB and this cannot be changed in code or XAML. This implementation does not have such limitations.

Checklist

Breaking changes

None

Obsoletions / Deprecations

None

Fixed issues

Closes #4179

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 0.10.999-cibuild0020723-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 0.10.999-cibuild0020724-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 0.10.999-cibuild0020731-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 0.10.999-cibuild0020732-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 0.10.999-cibuild0020733-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 0.10.999-cibuild0020734-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

@robloo
Copy link
Contributor Author

robloo commented Jul 8, 2022

@maxkatz6 A full review was done of the bool properties. Since this is finalizing the API and different from WinUI a bit more time was spent on this. Please let me know if you (or anyone else reading this) agree or have questions/concerns.

The main column to look at is the "New" one on the right. Some alternative ideas are listed to the immediate right of the "New" column but were decided against.

image

  1. "Section" could be added to better differentiate the tab visibility properties. "Section" is more generic than "Tab" but still may not be relevant in future re-templated controls.
  2. "IsComponentTextInputVisible" could be shortened to "IsComponentInputVisible". This matches better with "IsHexInputVisible"; however, may then be confused with "IsComponentSliderVisible" which is also another input.
  3. Both "IsColorModelVisible" and "IsColorPreviewVisible" need to keep the term "Color".
  4. Both "IsComponentsVisible" and "IsAccentColorsVisible" use the plural term which sounds a bit strange and may be grammatically incorrect. However, this is sometimes done in .NET to follow the "Is" convention.

Edit:

The name of the primitive is ColorSpectrum and the interface IColorPalette. So it makes sense to keep the existing color term in the name for those two (IsColorSpectrumVisible and IsColorPaletteVisible). IsColorSpectrumVisible also now matches WinUI again. Then to be consistent, IsColorComponentsVisible should be used for the third tab. So I ended up only changing ShowAccentColors to IsAccentColorsVisible (all others use "visible" and "is" terminology).

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 0.10.999-cibuild0021737-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 0.10.999-cibuild0021773-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 0.10.999-cibuild0021775-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 0.10.999-cibuild0021785-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

@maxkatz6
Copy link
Member

Personally, I don't have anything against of current naming in this PR, seems reasonable.
Details like Border#TabBackgroundBorder corner radius can be revisited later with ControlThemes.

@maxkatz6 maxkatz6 enabled auto-merge July 13, 2022 05:51
@robloo
Copy link
Contributor Author

robloo commented Jul 13, 2022

Well, I thought about the property naming some more. The name of the primitive is ColorSpectrum and the interface IColorPalette. So yes, it makes sense to keep the existing color term in the name for those two. Then to be consistent, ColorComponents should be used for the third tab. So I might end up only changing ShowAccentColors to IsAccentColorsVisible (all others use "visible" and "is" terminology).

I found another thing where the color previewed drop shadow should really be removed when accent colors are not shown.

If you could pause auto-merge I'll try to make the changes within the next few days.

@maxkatz6 maxkatz6 disabled auto-merge July 13, 2022 14:10
@maxkatz6
Copy link
Member

@robloo paused

@robloo
Copy link
Contributor Author

robloo commented Jul 15, 2022

@maxkatz6 Thanks, updates are made and this should be all set for now.

I will make the next round of updates converting to ControlTheme after those PRs are finalized and merged. Although, it still isn't clear to me how to really extend FluentTheme at that point.

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 0.10.999-cibuild0022103-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

@robloo
Copy link
Contributor Author

robloo commented Jul 15, 2022

Not sure what conflicts it is talking about unless it means the IntegrationTests failures. I merged in master branch with no issue.

I'm assuming the IntegrationTests are still a WIP? Let me know how to proceed here.

@maxkatz6
Copy link
Member

@robloo framework used for integration tests is very unstable, and it fails quite frequently. It's not yet solved, but PRs can be merged anyway, as it doesn't block merging. I can do it now, if you want to continue with control themes in another PR.

@robloo
Copy link
Contributor Author

robloo commented Jul 15, 2022

I can do it now, if you want to continue with control themes in another PR.

Yes, that was my plan and I'm not sure when the other PRs will be completed. If you could merge this soon that would be great. Sorry for not being more clear about it.

@maxkatz6 maxkatz6 merged commit 98ca548 into AvaloniaUI:master Jul 15, 2022
@robloo robloo deleted the colorpicker2 branch July 15, 2022 05:14
@timunie
Copy link
Contributor

timunie commented Jul 15, 2022

@robloo very beautiful and powerful color picker 👍 Can't wait to use it in v11.0 (I know I can use nightly builds, but they are breaking other things for me atm)

I think we now should create the needed docs for it. I can support you here if you want me to.

@robloo
Copy link
Contributor Author

robloo commented Jul 16, 2022

@timunie Thanks! Hope you find it useful.

I think we now should create the needed docs for it. I can support you here if you want me to.

I haven't forgotten about this and already started a draft of some ideas some time ago. However, there is no rush to complete it. As you know new docs are getting merged in right now.

I will plan on finishing this up myself due to the nature of this control. Your review after the initial draft will be very useful though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding ColorPicker and Related Primitive Controls
4 participants