-
Notifications
You must be signed in to change notification settings - Fork 632
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
6893: DYN-6956 symbol nodes should not show in HomeWorkspace context. #15204
6893: DYN-6956 symbol nodes should not show in HomeWorkspace context. #15204
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-6893
UI Smoke TestsTest: success. 2 passed, 0 failed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -194,6 +195,9 @@ | |||
|
|||
refreshLibraryView(libController); | |||
|
|||
//initial state | |||
libController.setHostContext("home") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Enzo707 think this line needs to be moved to librarie.js->library.html file once you consume the npm package inside Dynamo.
{ | ||
var type = workspace switch | ||
{ | ||
HomeWorkspaceModel _ => "home", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you defined the next line in librarie.js
export type HostingContextType = "home"|"custom"|"none"
What happens if is set to "none"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
custom and none do nothing special at the moment - library is rendered as before my prs.
/// <param name="type"></param> | ||
internal void UpdateContext(string type) | ||
{ | ||
ExecuteScriptFunctionAsync(browser,"libController.setHostContext", type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually we were defining the function in the library.html (like an interface of Dynamo and librarie.js), for example:
function SetContext(type){
libController.setHostContext(type)
}
But I guess this way is better if now everything will be inside the librarie.js component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually had trouble with that pattern where I found libController
was not set correctly, I guess I could figure out how to pass it as a arg to make sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like that we're using ExecuteScriptFunctionAsync for so many interops between c# and javascript. ExecuteScriptFunctionAsync forces you to write the javascript code on the c# side (prone to errors)
There are other ways to interop (post simple messages or hostObjects) We should review the interop approach in a separate task
src/LibraryViewExtensionWebView2/Handlers/NodeItemDataProvider.cs
Outdated
Show resolved
Hide resolved
the test failure exists on master already, merging. |
Purpose
relies on:
DynamoDS/librarie.js#228
Dynamo now informs librarieJS of the workspace context, when loadedTypes that have
hiddenInWorkspaceContext == true
are rendered and the context ishome
- they are simply not rendered on the JS side.Declarations
Check these if you believe they are true
*.resx
filesRelease Notes
fix a legacy issue where input and output symbol nodes were shown in the HomeWorkspace context even though they are only useful in CustomNodeWorkspaces.
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