-
Notifications
You must be signed in to change notification settings - Fork 184
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
Fix Vue wrapper packaging issue #236
Fix Vue wrapper packaging issue #236
Conversation
Deploy preview for carbon-charts ready! Built with commit 9b69345 |
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.
Hi,
When released, the JS files are not in the dist directory anymore, but in root:
Also, did you see #231 (comment)? Would adding the ./src
directory to the published package fix that? We should also have a bundle that includes everything in itself without the need of the ./src
folder
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 good generally 👍 just a couple tweaks due to some build/deploy changes we've implemented
@moores2 We need to modify |
from my comment here: #231 (comment) do we need so ship |
Yeah it would be preferable if we have a bundle that exports modules separately, and one that includes all where dev can use it through CDNs |
0e38217
to
9b69345
Compare
I have reworked index.js with a simpler approach, and added exports so that the source is not needed in the shipped package. This update should allow clients to load the wrappers in 2 ways: Firstly, to just load all of the chart components globally into Vue by using a Vue plug-in, like this:
The second option is to allow consumers to selectively import just the wrappers/components that they want, like this:
|
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.
Awesome! 🎉 LGTM
@moores2 @cal-smith This looks good 💯 |
Nope we're good to go 👍 |
Merging |
Fixes #231
main
for package correctlyNote: the vue-cli library build generates and extraneous dist/demo.html that does not work. There is an issue for this here: vuejs/vue-cli#3291
Review checklist (for reviewers only)