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

remove nodehelp control #11660

Merged
merged 2 commits into from
May 6, 2021
Merged

remove nodehelp control #11660

merged 2 commits into from
May 6, 2021

Conversation

mjkkirschner
Copy link
Member

@mjkkirschner mjkkirschner commented Apr 30, 2021

Purpose

When markdown docs for nodes were added to the docs browser - the old NodeHelpPrompt control was no longer displayed. It's not used anywhere in Dynamo today and likely will not be.

This PR just obsoletes it, so we get rid of it later at a major release.

This PR removes the control, but leaves the test as it doesn't really have anything to do with the control - it tests a function on NodeModel and still passes.

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

@mjkkirschner mjkkirschner changed the title obsolete old control obsolete nodehelp control Apr 30, 2021
@mjkkirschner
Copy link
Member Author

@Astul-Betizagasti noted that currently they are obsoleting the old controls, so I think it's fine to do the same here for now until we discuss API surface rules.

@RobertGlobant20
Copy link
Contributor

@mjkkirschner, related about what you were saying about WPF Controls (not sure if you are just talking about Properties in the ViewModel or controls in the XAML file), some days ago I created a new pull request for enabling the Geometry Scaling and Render Precision features in the Preferences panel, so I deleted the old code related to the Geometry Scaling functionality (xaml, code behind, events, commands, methods .....), is that ok? or should I marked as Obsolete?
This is the pull request:
#11657

Thanks
cc: @Astul-Betizagasti

@mjkkirschner
Copy link
Member Author

@RobertGlobant20 - I'm not sure - I think the safest thing to do for now is to obsolete anything public, add a TODO and we should discuss with @QilongTang how to approach it.

If we are going to start treating public methods and classes in DynamoCoreWPF as excluded from our API we should message this out, and at the minimum it must be documented it in the github readme or dev docs.
AFAIK we do treat some extensions like this already - but it's not consistent or documented well.

for this PR - that was what I was going to do -

  1. obsolete the unused controls for now.
  2. file a task to remove them at 3.x time in any case.
  3. discuss with teams if we want to create a new policy and how we should message it.

@QilongTang
Copy link
Contributor

Hey @mjkkirschner What we used to do was that:

Currently, we are treating DynamoView out of API scope, which means we can move/delete XML wpf controls as we want. For any public properties exposed under DynamoViewModel, we were only obsoleting them.

If you have any preference, feel free to update here or on slack conversation: https://autodesk.slack.com/archives/C01C2AGPSG2/p1620055650102300

@mjkkirschner
Copy link
Member Author

mjkkirschner commented May 5, 2021

@QilongTang okay - will proceed same as Morpheus team, filed a task for the messaging.

@josh2kand8

This comment was marked as spam.

keep test for dictionary
@mjkkirschner mjkkirschner merged commit 8f3af94 into DynamoDS:master May 6, 2021
@mjkkirschner mjkkirschner deleted the dyn3306 branch May 6, 2021 14:27
@mjkkirschner mjkkirschner changed the title obsolete nodehelp control remove nodehelp control Jun 4, 2021
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.

4 participants