Skip to content

Conversation

@ZiyunShang
Copy link
Collaborator

@ZiyunShang ZiyunShang commented Oct 20, 2020

…ntType"

Purpose

Rename Element Types node to make it more meaningful and distinguished from "ElementType"
Related to issue #2587

Declarations

Check these if you believe they are true

  • The code base 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
  • Snapshot of UI changes, if any.

Reviewers

@ShengxiZhang @wangyangshi

FYIs

@Amoursol @QilongTang @mjkkirschner

}

[NodeName("Element Types")]
[NodeName("Element Classes")]
Copy link
Collaborator Author

@ZiyunShang ZiyunShang Oct 20, 2020

Choose a reason for hiding this comment

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

This class ElementTypes is subclass of AllChildrenOfType<Element>, and can't modify its output name.
The output name is set in DynamoCore - https://github.com/DynamoDS/Dynamo/blob/master/src/Libraries/CoreNodeModels/Enum.cs#L65 .
So I think I need submit a update here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ElementClass
Hi @ShengxiZhang , with this PR, the results as the screenshot shown. We can't rename the output name "Type" on DynamoRevit side.

@mjkkirschner
Copy link
Member

I think it's important that before making this change testing is done on graphs and custom nodes that reference these nodes to make sure they open okay.

@ZiyunShang
Copy link
Collaborator Author

@mjkkirschner I just run all DynamoRevit tests and all have passed. I also test the old script with this change, it can run without error or warning.
test

@chuongmep
Copy link

I think we not necessary change name to class because engineer difficult for read document update .

@ZiyunShang
Copy link
Collaborator Author

I think we not necessary change name to class because engineer difficult for read document update .

But this does mislead some users and confuse them like #2587 . So I think this will be helpful for distinguishing Element Class and ElementType.

@ZiyunShang ZiyunShang merged commit 12c0d0a into DynamoDS:master Dec 30, 2020
@ZiyunShang ZiyunShang deleted the ReNameElementTypes branch December 30, 2020 09:02
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