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

Dyn 4738 custom color picker #13794

Merged
merged 6 commits into from
Mar 6, 2023
Merged

Dyn 4738 custom color picker #13794

merged 6 commits into from
Mar 6, 2023

Conversation

RobertGlobant20
Copy link
Contributor

Purpose

Replacing the current ColorPicker and ColorPalette used for GroupStyles and for the Color Palette node.
Currently in Dynamo the GroupStyles are using the System.Windows.Forms.ColorDialog control for selecting the color and the Color Palette node is using the WPF Extended Toolkit ColorPicker, now with this change in both places we will be using the CustomColorPicker (based in WPF Extended Toolkit ColorPicker).

Basically I did the next tasks:

  • Remove WPF Extended Toolkit 3.0.0 from CoreNodeModelsWpf project.
  • Add WPF Extended Toolkit 3.0.0 to DynamoCoreWpf project
  • Create a new ColorPicker named CustomColorPicker (based in WPF Extended Toolkit ColorPicker).
  • Use the new ColorPicker in Color Palette node.
  • Use the new ColorPicker for selecting the color when creating new styles (Preferences -> GroupStyles).

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated

Release Notes

Replacing the current ColorPicker and ColorPalette used for GroupStyles and for the Color Palette node.

Reviewers

@QilongTang

FYIs

@zeusongit

@RobertGlobant20
DYN-4738-Custom-ColorPicker
1c2dcfa 
First version of the customized ColorPicker so it can be used in GroupStyles and in Color Palette node.
@RobertGlobant20
DYN-4738-Custom-ColorPicker
34ec07a 
CustomColorPicker fixed to use only the Basic Colors and hide the Custom Colors.
First changes for integrating color picker in Color Palette node.
@RobertGlobant20
DYN-4738-Custom-ColorPicker
8773162 
Adapting the CustomColorPicker to be used in the Color Palette node.
@RobertGlobant20
DYN-4738-Custom-ColorPicker
215ed6c 
Refactoring some code and adding comments.
@RobertGlobant20
Copy link
Contributor Author

This is a GIF showing the behavior of the CustomColorPicker in both places:
CustomColorPicker

@zeusongit
Copy link
Contributor

From the GIF I see that there is considerable lag when you open the color picker for the first time, is it because you are in Debug, or is it usual?

@Amoursol
Copy link
Contributor

Amoursol commented Mar 2, 2023

@RobertGlobant20 looks like we're missing the "Advanced" section that allows users to select and specify their own color in full RGB space. Not having this would be a pretty significant downgrade on existing ability to select colors.

image
image

@Amoursol
Copy link
Contributor

Amoursol commented Mar 2, 2023 •
edited
Loading

@RobertGlobant20 we should also update the style of the base node here too, as the border and dropdown are of the old style.

  1. Ideally the color picker is centrally aligned with the output port, ideally the same full height.
  2. Down chevron icon, style and color scheme of button to match other Visual refresh elements
  3. Border of color selection to match other visual refresh elements

CC @Jingyi-Wen

image

@RobertGlobant20
Copy link
Contributor Author

@RobertGlobant20 looks like we're missing the "Advanced" section that allows users to select and specify their own color in full RGB space. Not having this would be a pretty significant downgrade on existing ability to select colors.

image image

@Amoursol The advanced section will be part of the next Jira task, right now that section is hidden (we decide it to do it in a different task due that is a complete different window with different style).
Thanks

@RobertGlobant20
Copy link
Contributor Author

From the GIF I see that there is considerable lag when you open the color picker for the first time, is it because you are in Debug, or is it usual?
@zeusongit well ... my computer (the old one) is extremely slow even loading the Library takes several seconds to show so I expect for normal users should be quicker. Thanks

@QilongTang QilongTang added this to the 2.18.0 milestone Mar 2, 2023
QilongTang
QilongTang reviewed Mar 2, 2023
View reviewed changes
src/DynamoCoreWpf/UI/Converters.cs Outdated
@@ -3641,4 +3641,60 @@ private double GetRelativeLuminance(System.Windows.Media.Color color)
return 0.2126 * R + 0.7152 * G + 0.0722 * B;
}
}

/// <summary>
/// This converter is used to add margin/Padding from Popup in the CustomColorPicker
Copy link
Contributor

Choose a reason for hiding this comment

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

add margin/Padding and return the added value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@QilongTang the comment was updated in the next commit: a49d970

@RobertGlobant20
Copy link
Contributor Author

From the GIF I see that there is considerable lag when you open the color picker for the first time, is it because you are in Debug, or is it usual?

@zeusongit
GIF updated using my new laptop:
AnimationColorPicker

QilongTang
QilongTang reviewed Mar 3, 2023
View reviewed changes
Copy link
Contributor

@QilongTang QilongTang left a comment

Choose a reason for hiding this comment

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

LGTM with comments

@QilongTang
Copy link
Contributor

QilongTang commented Mar 6, 2023 •
edited
Loading

Once the last comment addressed, good to go. @RobertGlobant20 Please also manul test adding group styles locally to see if the data in settings file is in a consistent format.

@RobertGlobant20
DYN-4738-Custom-ColorPicker Code Review2
e3416c6 
Reverting code added for debugging.
@RobertGlobant20
Copy link
Contributor Author

Once the last comment addressed, good to go. @RobertGlobant20 Please also manul test adding group styles locally to see if the data in settings file is in a consistent format.

@QilongTang here is a GIF of how the GroupStyles are serialized in DynamoSettings.xml file.
TestSerializerColorPicker

@QilongTang QilongTang merged commit 4e5a9a2 into DynamoDS:master Mar 6, 2023
@mjkkirschner mjkkirschner mentioned this pull request Mar 6, 2023
Merged
8 tasks
sm6srw pushed a commit to sm6srw/Dynamo that referenced this pull request Mar 29, 2023
@RobertGlobant20 @sm6srw
Dyn 4738 custom color picker (DynamoDS#13794)
57b41c5 
* DYN-4738-Custom-ColorPicker

First version of the customized ColorPicker so it can be used in GroupStyles and in Color Palette node.

* DYN-4738-Custom-ColorPicker

CustomColorPicker fixed to use only the Basic Colors and hide the Custom Colors.
First changes for integrating color picker in Color Palette node.

* DYN-4738-Custom-ColorPicker

Adapting the CustomColorPicker to be used in the Color Palette node.

* DYN-4738-Custom-ColorPicker

Refactoring some code and adding comments.

* DYN-4738-Custom-ColorPicker Code Review1

Adding comments

* DYN-4738-Custom-ColorPicker Code Review2

Reverting code added for debugging.
sm6srw pushed a commit that referenced this pull request Apr 5, 2023
@RobertGlobant20 @sm6srw
Dyn 4738 custom color picker (#13794)
9030ede 
* DYN-4738-Custom-ColorPicker

First version of the customized ColorPicker so it can be used in GroupStyles and in Color Palette node.

* DYN-4738-Custom-ColorPicker

CustomColorPicker fixed to use only the Basic Colors and hide the Custom Colors.
First changes for integrating color picker in Color Palette node.

* DYN-4738-Custom-ColorPicker

Adapting the CustomColorPicker to be used in the Color Palette node.

* DYN-4738-Custom-ColorPicker

Refactoring some code and adding comments.

* DYN-4738-Custom-ColorPicker Code Review1

Adding comments

* DYN-4738-Custom-ColorPicker Code Review2

Reverting code added for debugging.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QilongTang QilongTang QilongTang left review comments

@zeusongit zeusongit zeusongit approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.18.0
Development

Successfully merging this pull request may close these issues.
4 participants
@RobertGlobant20 @zeusongit @Amoursol @QilongTang