-
Notifications
You must be signed in to change notification settings - Fork 23
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
Feature/prepare librariejs bundle and resources #229
Feature/prepare librariejs bundle and resources #229
Conversation
Quality Gate passedIssues Measures |
Ready for review @QilongTang PTAL, thanks |
Do you think these changes would affect the layout spec for our integrators? e.g. https://github.com/DynamoDS/DynamoRevit/blob/master/src/DynamoRevit/Resources/LayoutSpecs.json |
@QilongTang I don't think it should but might be wise to ask others with more background than I to take a look just to be sure. Because as we did in Dynamo we could have added code that strongly depends on current implementation |
I am pretty sure that the layout spec in this repo (and of course the loaded types), should not be copied over to Dynamo. They are not accurate to what is in Dynamo. The layout spec and types in this repo are loaded just for the debug app that is served by the express server. The Revit layout spec is applied on top of the layout spec in Dynamo. I'm not sure how your changes might affect that but TLDR - don't copy the layout spec or loaded types from this repo over the ones in Dynamo when it gets integrated there. |
@@ -167,7 +167,7 @@ export class LibraryItem extends React.Component<LibraryItemProps, LibraryItemSt | |||
<div className="LibraryItemIconWrapper"> | |||
<img | |||
className={"LibraryItemIcon"} | |||
src={this.props.data.iconUrl} | |||
src={require("../"+this.props.data.iconUrl)} |
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.
so in the context of a browser, and not node, what does this do when transpiled?
Maybe another way of asking this question - why is this required?
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.
so in the context of a browser, and not node, what does this do when transpiled? Maybe another way of asking this question - why is this required?
This allows webpack to get the resource and include it into the build
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.
How does web pack include these images in the build? I think you might find that on the Dynamo side we do some odd things to load/embed images as base64 data.
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.
librariejs generates the build with webpack including the bundle, index.html and the resources which are served using express for debug purposes.
On Dynamo side we should load the build as we do in NotificationsCenter but as you mention there are odd things to handle resources. I'm debugging LibraryViewController now, and seems like we replace the resources url in the bundle according to Dynamo's current LayoutSpects.json
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.
Thank you for the explanation!
Hi @Enzo707 Should we close this PR after our discussions? |
Review
@QilongTang
FYI
@mjkkirschner