DynamoDS
AGD-2199
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
- [x] The codebase is in a better state after this PR
- [ ] Is documented according to the standards
- [x] The level of testing this PR includes is appropriate
- [ ] User facing strings, if any, are extracted into
*.resxfiles - [x] All tests pass using the self-service CI.
- [x] 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
@LongNguyenP @saintentropy There is a regression NodeDocumentationMarkdownGeneratorTests.MarkdownGeneratorCommandTests.ProducesCorrectOutputFromCoreDirectory
Hi Long, I added some change requests, in addition please also the following:
- tests for the new node and new classes if possible.
- add an image or gif of the node in action to this PR
Also curious, @Jingyi-Wen @LongNguyenP Is there a new icon planned for this node?
Two ideas for the icon (grey background is just for reviewing visibility)
@mjkkirschner @LongNguyenP
@mjkkirschner @saintentropy Michael. Craig and I are thinking of making another PR for the tests so that this PR can go ahead without getting too big. Would this be a reasonable way forward?
@mjkkirschner @saintentropy. I fixed a regression bug, added a couple of tests to ensure the saved menu items and selected items are saved and opened correctly from the dyn file. Also change quite a few public methods and to private to be as conservative as possible.
@LongNguyenP will review when I am able - but there are 3 test failures: https://master-5.jenkins.autodesk.com/job/Dynamo/job/DynamoSelfServe/job/pullRequestValidation/5401/
@mjkkirschner I think this is ready to go :-) with the follow tasks mentioned
@saintentropy @LongNguyenP can you link the jira tickets of the followup tasks?
https://jira.autodesk.com/browse/DYN-4795 and https://jira.autodesk.com/browse/DYN-4796 @mjkkirschner for the follow up tasks
@QilongTang Craig suggested I bring this PR to your attention. I've made some modifications to make better use of existing UI and styles. There are a few changes outside of this component, mostly to enable better sharing and style inheritance, namely in DynamoModern.xaml. See my comments there. Happy to discuss!
@twastvedt @saintentropy I'm less worried about the style changes. While this is a new node I see, I highly encourage you to add an example of how it works as part of our node help docs. There are plenty examples at https://github.com/DynamoDS/Dynamo/tree/master/doc/distrib/NodeHelpFiles.
And consider the real effort is to
- add some description
- add an image of a small block of an example graph illustrating how it works
- name the files following how other files are named, so that the context menu of nodes would locate your doc OOTB
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.