-
-
Notifications
You must be signed in to change notification settings - Fork 62
Support for embedding source in kicanvas-source #126
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
base: main
Are you sure you want to change the base?
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.
Looks mostly good with one small comment!
return; | ||
} | ||
|
||
await this.#setup_project( |
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.
instead of three separate invocations of await this.#setup_project
, have the preceding code construct a vfs
and then have the last statement be:
return await #this.setup_project(vfs);
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.
good call, done!
continue; | ||
} | ||
|
||
let filename = src_elm.name?.trim(); |
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.
Because querySelectorAll
is used, the custom element property is not reliably accessible and may return undefined at runtime. To address this problem, getAttribute
should be used on the returned element.
I've changed this to src_elm.getAttribute('name')?.trim()
in my local copy to resolve the issue.
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.
Interesting - I'm wondering if this is executing before the element has finished being registered?
|
||
let filename = src_elm.name?.trim(); | ||
if (!filename || filename.length == 0) { | ||
const type = (src_elm.type ?? "schematic").toLowerCase(); |
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.
Similarly to the comment above I've changed this to src_elm.getAttribute('type')
in my local copy to resolve the issue.
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've tested this change as it's useful for my own project, and found that at runtime the code won't have the name
and type
custom properties reliably available when accessing elements through querySelectorAll
. To address the problem it is recommended to use getAttribute
to access the two custom attributes.
No description provided.