AGD-2199 - custom dropdown node #11958

LongNguyenP · 2021-08-23T15:50:20Z

Purpose

This PR addresses JIRA task AGD-2199: A node that allows user to pre-define a dropdown menu with an arbitrary number of customizable menu items

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

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(FILL ME IN, optional) Any additional notes to reviewers or testers.

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

src/Libraries/CoreNodeModels/Properties/Resources.en-US.resx

…2199

Co-authored-by: Craig Long <[email protected]>

* Fix broken tests * Remove temp test files Co-authored-by: Craig Long <[email protected]>

…2199 � Conflicts: � src/Libraries/CoreNodeModels/Properties/Resources.en-US.resx � src/Libraries/CoreNodeModels/Properties/Resources.resx

QilongTang · 2021-12-08T02:12:40Z

@LongNguyenP @saintentropy There is a regression NodeDocumentationMarkdownGeneratorTests.MarkdownGeneratorCommandTests.ProducesCorrectOutputFromCoreDirectory

# Conflicts: # src/AssemblySharedInfoGenerator/AssemblySharedInfo.cs

LongNguyenP · 2022-01-31T15:48:55Z

Merge conflicts with the current master has been resolved. LGTM

src/Libraries/CoreNodeModels/CustomSelectionItem.cs

…D-2199

doc/distrib/NodeHelpFiles/CoreNodeModels.Input.DoubleInput.md

* Update CoreUtils.cs * add test

…D-2199

mjkkirschner · 2022-08-18T17:03:03Z

doc/distrib/NodeHelpFiles/CoreNodeModels.Input.CustomSelection.md

@@ -0,0 +1,6 @@
+## In Depth
+The Custom Dropdown node allows a user to create a dropdown selection input with custom labels and values. If all values are numbers, the output will be a double, and if all values are integers the output will be an integer. In the example below, "Two" is selected in the Custom Dropdown Menu node, making the output of that node the integer `2`. 


what happens if the types are mixed? Can you use strings? What are valid value types etc?

Is this dynamic behavior important - I ask this because as we're working on the MSIL compiler - it's very hard to determine the output types of nodes that have dynamic behavior like this - not super important but I imagine that if we move forward with that work outside of a POC we're going to need some principles around not introducing dynamic behavior without a really good reason.

Immediate answers: values default to strings unless the criteria are met. So, anything's valid, it will just be a string unless we can make everything numbers. I'll add that, or whatever we decide here, to the description.

I'm definitely open to alternatives, though I think this behavior is pretty convenient, intuiting a user's expectations in most common scenarios. I'm not sure what the alternative to dynamic behavior here is. Separate String custom selection and Number custom selection nodes sounds awkward for the user. I imagine the original inspiration for this part may have been the experience in Grasshopper. Though there it's less a feature of the custom selection node itself and more of the global type coercion.

let's put this on the back burner.

src/AssemblySharedInfoGenerator/AssemblySharedInfo.tt

mjkkirschner · 2022-08-18T17:04:33Z

src/DynamoCoreWpf/UI/Themes/Modern/DynamoModern.xaml

                    xmlns:dynui="clr-namespace:Dynamo.UI.Controls;assembly=DynamoCoreWpf"
                    xmlns:fa="clr-namespace:FontAwesome.WPF;assembly=FontAwesome.WPF"
-                    xmlns:nodes="clr-namespace:Dynamo.Nodes;assembly=DynamoCore"


so was this just incorrect?

ah. The removed line was unused. I needed a reference to DynamoCoreWpf's Dynamo.Nodes, which still made sense to call nodes.

mjkkirschner · 2022-08-18T17:05:01Z

src/DynamoCoreWpf/UI/Themes/Modern/DynamoModern.xaml

@@ -1414,61 +1414,53 @@
    </Style>

    <Style x:Key="SNodeTextButton" TargetType="{x:Type Button}">
+        <Setter Property="FontSize" Value="17px"/>
+        <Setter Property="Foreground" Value="{StaticResource Blue300Brush}" />
+        <Setter Property="Background" Value="#4D4D4D" />


is this color reused in a lot of places? Can it be pulled out?

Looks like it's used in 5 places in DynamoModern. It already exists in ColorsAndBrushes, but for more specific uses. I wasn't sure how far to take a style revision, so I defaulted to just making the changes necessary to use the styles for the new component. From my interaction so far it looks like there are a lot of optimization opportunities like this: unnecessary styles, duplicated sections, hard-coded colors.

So, yes, definitely could be, just let me know if you want that level of change to be in this PR

mjkkirschner · 2022-08-18T17:05:21Z

src/DynamoCoreWpf/UI/Themes/Modern/DynamoModern.xaml

                                       FontWeight="Bold"
                                       Style="{StaticResource SZoomFadeText}"
                                       Text="{Binding RelativeSource={RelativeSource TemplatedParent}, Path=Content}" />
                        </Border>
                    </Grid>
                    <ControlTemplate.Triggers>
                        <Trigger Property="Button.IsMouseOver" Value="true">
-                            <Setter TargetName="roundedBorder" Property="Background" Value="#535353" />
-                            <Setter TargetName="text" Property="Foreground" Value="{StaticResource Blue300Brush}" />
+                            <Setter Property="Background" Value="#535353" />


same color question

A popular color used in 7 places in DynamoModern, already has a color in ColorsAndBrushes, and used by 3 different brushes in ColorsAndBrushes. Also used directly in 2 other view files.

mjkkirschner · 2022-08-18T17:06:17Z

src/DynamoCoreWpf/UI/Themes/Modern/DynamoModern.xaml

+    <Style TargetType="{x:Type nodes:DynamoNodeButton}" BasedOn="{StaticResource SNodeTextButton}" />
+
+    <Style x:Key="SingleCharButton" TargetType="{x:Type Button}" BasedOn="{StaticResource SNodeTextButton}">
+        <Setter Property="Foreground" Value="#999999" />


color reuse question

src/Libraries/CoreNodeModels/Input/CustomSelection.cs

mjkkirschner · 2022-08-18T17:11:17Z

src/Libraries/CoreNodeModels/Input/CustomSelection.cs

+            return SelectionState.Restore;
+        }
+
+        [OnSerializing]


what does this attribute do?

This is a Newtonsoft pre-serialization hook. https://www.newtonsoft.com/json/help/html/SerializationCallbacks.htm

mjkkirschner

a few questions / comments

src/Libraries/CoreNodeModelsWpf/Controls/CustomSelectionControl.xaml.cs

mjkkirschner · 2022-08-18T17:14:34Z

src/Libraries/CoreNodeModelsWpf/Controls/CustomSelectionControl.xaml.cs

+
+            if (e.Key == Key.Tab)
+            {
+                int offset = e.KeyboardDevice.Modifiers == System.Windows.Input.ModifierKeys.Shift ? 0 : 1;


I am usually not a stickler for style stuff besides if(){ } - but the whitespace seems a bit confusing to the eye.

More than happy to oblige. I miss Prettier. Is this what you had in mind?

src/Libraries/CoreNodeModelsWpf/CustomSelectionViewModel.cs

src/Libraries/CoreNodeModelsWpf/NodeViewCustomizations/CustomSelection.cs

mjkkirschner · 2022-08-18T17:18:28Z

src/LibraryViewExtensionMSWebBrowser/web/library/layoutSpecs.json

@@ -443,6 +443,9 @@
                },
                {
                  "path": "Core.Input.Output"
+                },
+                {
+                  "path": "Core.Input.Custom Dropdown Menu"


I won't die on this hill, but is this really a dropdown menu?

Nope, nope it isn't. I had provisionally renamed things to "Custom Selection", but missed this one. While we're here though, is that the right name? "Custom Dropdown"? "Custom Selection List"? I guess I'd still vote "Custom Selection", but I won't even risk injury on that hill.

test/Tools/docGeneratorTestFiles/sampledictionarycontent/Dynamo_Nodes_Documentation.json

mjkkirschner · 2022-08-18T17:25:43Z

also kicked off: https://master-15.jenkins.autodesk.com/view/DYN/job/DYN-DevCI_Self_Service/1083/ on the serial tests just to see if anything unexpected happens there.

tests pass there - another good practice would be to pull master into your branch, this is already done on the parallel tests, but for the serial tests, it's not done with the job I linked above, so we could still find some unexpected behavior on master-15 when we merge. I don't think it's necessary here, but good for next time.

twastvedt

Thanks for the review! I've fixed most things (I think!), a few might be worth further discussion.

twastvedt · 2022-08-18T19:38:17Z

src/DynamoCoreWpf/UI/Themes/Modern/DynamoModern.xaml

                                       FontWeight="Bold"
                                       Style="{StaticResource SZoomFadeText}"
                                       Text="{Binding RelativeSource={RelativeSource TemplatedParent}, Path=Content}" />
                        </Border>
                    </Grid>
                    <ControlTemplate.Triggers>
                        <Trigger Property="Button.IsMouseOver" Value="true">
-                            <Setter TargetName="roundedBorder" Property="Background" Value="#535353" />
-                            <Setter TargetName="text" Property="Foreground" Value="{StaticResource Blue300Brush}" />
+                            <Setter Property="Background" Value="#535353" />


A popular color used in 7 places in DynamoModern, already has a color in ColorsAndBrushes, and used by 3 different brushes in ColorsAndBrushes. Also used directly in 2 other view files.

src/Libraries/CoreNodeModels/Input/CustomSelection.cs

twastvedt · 2022-08-18T19:43:10Z

src/Libraries/CoreNodeModels/Input/CustomSelection.cs

+            return SelectionState.Restore;
+        }
+
+        [OnSerializing]


This is a Newtonsoft pre-serialization hook. https://www.newtonsoft.com/json/help/html/SerializationCallbacks.htm

src/Libraries/CoreNodeModelsWpf/Controls/CustomSelectionControl.xaml.cs

twastvedt · 2022-08-18T19:53:49Z

src/Libraries/CoreNodeModelsWpf/Controls/CustomSelectionControl.xaml.cs

+
+            if (e.Key == Key.Tab)
+            {
+                int offset = e.KeyboardDevice.Modifiers == System.Windows.Input.ModifierKeys.Shift ? 0 : 1;


More than happy to oblige. I miss Prettier. Is this what you had in mind?

src/Libraries/CoreNodeModelsWpf/CustomSelectionViewModel.cs

src/Libraries/CoreNodeModelsWpf/NodeViewCustomizations/CustomSelection.cs

twastvedt · 2022-08-18T20:10:30Z

src/LibraryViewExtensionMSWebBrowser/web/library/layoutSpecs.json

@@ -443,6 +443,9 @@
                },
                {
                  "path": "Core.Input.Output"
+                },
+                {
+                  "path": "Core.Input.Custom Dropdown Menu"


Nope, nope it isn't. I had provisionally renamed things to "Custom Selection", but missed this one. While we're here though, is that the right name? "Custom Dropdown"? "Custom Selection List"? I guess I'd still vote "Custom Selection", but I won't even risk injury on that hill.

src/Libraries/CoreNodeModelsWpf/CustomSelectionViewModel.cs

test/Tools/docGeneratorTestFiles/sampledictionarycontent/Dynamo_Nodes_Documentation.json

mjkkirschner

Thanks @twastvedt looks good to me.

LongNguyenP added 3 commits August 12, 2021 14:02

First successful build

2fdcfcc

Added to Library View Extension

30e278d

Remove dependency on PresentationCore.dll

c0553ff

LongNguyenP requested review from saintentropy and nate-peters August 23, 2021 15:50

Updated some attributes

00587d3

saintentropy reviewed Sep 7, 2021

View reviewed changes

src/Libraries/CoreNodeModels/Properties/Resources.en-US.resx Outdated Show resolved Hide resolved

LongNguyenP and others added 10 commits September 14, 2021 16:20

Added some resource strings

37f164b

AssemblySharedInfo

795dfd7

Changed name to "Custom Dropdown"

d15bfd1

Merge branch 'master' of https://github.com/DynamoDS/Dynamo into AGD-…

975b821

…2199

Added MD file

047b9e2

Made changes to address two failed tests

4c843b2

Fix broken tests (#12082)

c05095f

Co-authored-by: Craig Long <[email protected]>

Added test related files

8b1d743

AGD 2199 Test Fix (#12161)

93377f7

* Fix broken tests * Remove temp test files Co-authored-by: Craig Long <[email protected]>

Merge branch 'master' into AGD-2199

b1ec208

QilongTang added this to the 2.13.0 milestone Nov 30, 2021

LongNguyenP added 2 commits December 1, 2021 15:58

Merge branch 'master' of https://github.com/DynamoDS/Dynamo into AGD-…

0960dd5

…2199 � Conflicts: � src/Libraries/CoreNodeModels/Properties/Resources.en-US.resx � src/Libraries/CoreNodeModels/Properties/Resources.resx

Modern styling applied to CustomDropdown node

9361301

QilongTang removed this from the 2.13.0 milestone Dec 8, 2021

saintentropy and others added 3 commits December 8, 2021 14:57

Update MarkdownGeneratorCommandTests.cs

a37cb5e

Merge branch 'master' into AGD-2199

93f4013

# Conflicts: # src/AssemblySharedInfoGenerator/AssemblySharedInfo.cs

Resolved PR conflicts with master

d6e00e2

LongNguyenP requested review from QilongTang, saintentropy and jasonstratton January 31, 2022 15:49

mjkkirschner reviewed Jan 31, 2022

View reviewed changes

src/Libraries/CoreNodeModels/CustomSelectionItem.cs Outdated Show resolved Hide resolved

Merge branch 'AGD-2199' of https://github.com/DynamoDS/Dynamo into AG…

0b11cd6

…D-2199

mjkkirschner reviewed Aug 16, 2022

View reviewed changes

doc/distrib/NodeHelpFiles/CoreNodeModels.Input.DoubleInput.md Show resolved Hide resolved

aparajit-pratap and others added 4 commits August 16, 2022 21:54

Treat null AST as primitive for update graph execution cycle (#13221)

27ccb15

* Update CoreUtils.cs * add test

Fix some tests.

752c90f

Merge branch 'AGD-2199' of https://github.com/DynamoDS/Dynamo into AG…

882f0b5

…D-2199

Merge branch 'master' into AGD-2199

223deee

mjkkirschner reviewed Aug 18, 2022

View reviewed changes

src/AssemblySharedInfoGenerator/AssemblySharedInfo.tt Outdated Show resolved Hide resolved

mjkkirschner reviewed Aug 18, 2022

View reviewed changes

mjkkirschner changed the title ~~AGD-2199~~ AGD-2199 - custom dropdown node Aug 18, 2022

twastvedt added 10 commits August 18, 2022 13:54

Revert template change.

f0b7445

Fix comment text.

86c439d

Localize output description.

7f74e9e

Add documentation.

1ba123d

Reduce property visibility.

ae35692

Fix inline if format.

7a5992b

Docs.

735d0c4

Reduce whitespace.

80ad1d1

Docs fix.

24e8014

Naming fixes.

45047e7

twastvedt reviewed Aug 18, 2022

View reviewed changes

mjkkirschner approved these changes Aug 19, 2022

View reviewed changes

twastvedt merged commit 4819d58 into master Aug 19, 2022

twastvedt deleted the AGD-2199 branch August 19, 2022 14:20

twastvedt mentioned this pull request Aug 30, 2022

Update node documentation image. #13270

Merged

1 task

QilongTang mentioned this pull request Jun 25, 2024

DYN-7162 Add back missing node help docs and fix cases were not working #15349

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AGD-2199 - custom dropdown node #11958

AGD-2199 - custom dropdown node #11958

LongNguyenP commented Aug 23, 2021 •

edited

Loading

QilongTang commented Dec 8, 2021

LongNguyenP commented Jan 31, 2022 •

edited

Loading

mjkkirschner Aug 18, 2022

twastvedt Aug 18, 2022

mjkkirschner Aug 19, 2022

mjkkirschner Aug 18, 2022

twastvedt Aug 18, 2022

mjkkirschner Aug 18, 2022

twastvedt Aug 18, 2022

twastvedt Aug 18, 2022

mjkkirschner Aug 18, 2022

twastvedt Aug 18, 2022

mjkkirschner Aug 18, 2022

mjkkirschner Aug 18, 2022

twastvedt Aug 18, 2022

mjkkirschner left a comment

mjkkirschner Aug 18, 2022

twastvedt Aug 18, 2022

mjkkirschner Aug 18, 2022

twastvedt Aug 18, 2022

mjkkirschner commented Aug 18, 2022 •

edited

Loading

twastvedt left a comment

twastvedt Aug 18, 2022

twastvedt Aug 18, 2022

twastvedt Aug 18, 2022

twastvedt Aug 18, 2022

mjkkirschner left a comment

		@@ -0,0 +1,6 @@
		## In Depth
		The Custom Dropdown node allows a user to create a dropdown selection input with custom labels and values. If all values are numbers, the output will be a double, and if all values are integers the output will be an integer. In the example below, "Two" is selected in the Custom Dropdown Menu node, making the output of that node the integer `2`.

AGD-2199 - custom dropdown node #11958

AGD-2199 - custom dropdown node #11958

Conversation

LongNguyenP commented Aug 23, 2021 • edited Loading

Purpose

Declarations

Reviewers

FYIs

QilongTang commented Dec 8, 2021

LongNguyenP commented Jan 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner commented Aug 18, 2022 • edited Loading

twastvedt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner left a comment

Choose a reason for hiding this comment

LongNguyenP commented Aug 23, 2021 •

edited

Loading

LongNguyenP commented Jan 31, 2022 •

edited

Loading

mjkkirschner commented Aug 18, 2022 •

edited

Loading