-
Notifications
You must be signed in to change notification settings - Fork 794
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
Add inline argument to chart.save() for html export #2807
Conversation
This includes the Vega/Vega-Lite/Vega-Embed JavaScript source in the exported html file so that it works offline. The logic is ported from altair_saver.
Seeing a couple of CI errors with altair_saver and Python 3.11. Does this look familiar to anyone? Should be try kicking off the tests again? |
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 reran the test and it is passing now. I think this looks good overall, just one or two questions in the comments. @mattijn does this approach makes sense to you too?
if template == "inline": | ||
try: | ||
from altair_viewer import get_bundled_script | ||
except ImportError: | ||
raise ImportError( | ||
"The altair_viewer package is required to convert to HTML with inline=True" | ||
) |
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 like this approach versus adding altair_viewer
as a required dependency for altair
This way it's still possible to pass in custom templates
Also, I tested this manually. But it would be good for someone else to try it out manually as well since the test case for checking that it's working just checks for an expected substring that's different that what's expected with |
Co-authored-by: Mattijn van Hoek <mattijn@gmail.com>
@jonmmease: out of curiosity: where does vl-convert get the javascript bundles of vega and vega-lite from? |
It pulls them from the Skypack CDN, which integrates well with Deno. The kind of interesting thing is that there's no global bundler, VlConvert inlines a bunch of individual bundles (for each dependency) and Deno looks them up dynamically on import. This is the directory of source files that are inlined into into VlConvert on build: https://github.com/vega/vl-convert/tree/main/vl-convert-rs/vendor. I was thinking about whether there would be a good way for vl-convert to inline these into a standalone HTML file, but I'm not sure how to manually bundle them so that imports work. I was thinking about whether there would be a good way to use these |
or how altair could use these if vl-convert-python is installed.. |
Regarding this PR, I can understand the intention, but I trust you here @joelostblom. I never have used this functionality in |
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 tested manually (using the 4.17.0 vega-lite version in altair viewer) and this works as expected.
I had one question, after this PR, can you still use the same functionality from altair_saver?
I tested both with and without altair_saver installed and the changes in this PR takes precedence so if you are using a version of altair from after this PR was merged, then you will not use the altair_saver functionality. However, I think it makes sense to keep that functionality around in altair saver to support older version of altair.
Merging, thanks @jonmmease !
Note: once Altair v5 is released, this feature will only work correctly after a new release of altair_viewer as well. Since the current release of altair_viewer (v0.4.0) does not include the JavaScript source of vega-lite v5, but just v4 (v4.17.0). |
This includes the Vega/Vega-Lite/Vega-Embed JavaScript source in the exported html file so that it works offline. The logic is ported from altair_saver.
See #2765
Note: Like altair_saver, this pulls the JavaScript source from the
altair_viewer
package. In the future we could talk about whether to include the JavaScript bundles in Altair itself to avoid this dependency.