-
Notifications
You must be signed in to change notification settings - Fork 44
Rewrite inject_dartpad and expose some functionality in library #247
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
I also plan to use this new support in the Jaspr version of the docs site, rather than the prebuilt script. Once that is complete, I'll remove the pre-built JS and surrounding compilation support. |
/// | ||
/// This ID is used both as the HTML element `id` and | ||
/// as the iframe's `name` attribute for message targeting. | ||
final String iframeId; |
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.
This makes it seem like the user needs to put an iframe on the page, but my understanding is that this needs to be a <code>
element, right? Maybe we should make this just elementId
if that's the case.
host.appendChild(iframe); | ||
parent.replaceWith(host); | ||
await embeddedDartPad.initialize( | ||
addToDocument: (iframe) { |
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.
maybe this should be named "onElementCreated" or similar
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, nice API!
The `inject_dartpad.dart.js` script needs to be inlined now that site-shared is not a submodule. However, this is temporary as we plan to replace this with a new Dart API directly from a Jaspr component. The API is added in dart-lang/site-shared#247.
Previously
web/inject_dartpad.dart
was just written as a script for dart.dev and docs.flutter.dev, but the logic wasn't really reusable in other contexts and didn't have much error handling. This PR rewrites and moves the core logic into a new library and class atlib/inject_dartpad.dart
that other clients can take advantage of through using this package as a git dependency.Besides being a public library, the new library also makes a few improvements over the original script.
jsify
with directly constructing the appropriate JS object.The script in
web/inject_dartpad.dart
was then updated to use the new library, with a few additional improvements:\cc @craiglabenz Since I can't assign you as a reviewer. I guess you don't have write access to this repo.