Conversation
gvwilson
left a comment
There was a problem hiding this comment.
Left a few comments and a question, happy to read through again as soon as you want.
plotly/io/_kaleido.py
Outdated
| - 'webp' | ||
| - 'svg' | ||
| - 'pdf' | ||
| - 'eps' (Requires the poppler library to be installed and on the PATH) |
There was a problem hiding this comment.
@gvwilson Following up on this — Kaleido v1 does not support EPS yet. So either we drop support for EPS entirely, or document that EPS is only available with Kaleido v0 and add an informative error message.
There was a problem hiding this comment.
please do the latter - thanks
| - "kaleido": Use Kaleido for image export | ||
| - "orca": Use Orca for image export | ||
| - "auto" (default): Use Kaleido if installed, otherwise use orca | ||
| engine (deprecated): str |
There was a problem hiding this comment.
do we print a deprecation warning if this argument is used?
There was a problem hiding this comment.
Not yet, assuming we are in agreement about removing this argument, I'll add one
There was a problem hiding this comment.
I'm in agreement that we should remove this argument
| def check_image(path_or_buffer, size=(700, 500), format="PNG"): | ||
| if format == "PDF": | ||
| img = PdfReader(path_or_buffer) | ||
| # TODO: There is a conversion factor needed here |
There was a problem hiding this comment.
Part of the TODO is for me to educate myself on how Plotly currently determines PDF size when writing to PDF. :)
But the size argument is measured in pixels (at least for raster file types) and PDFs have no concept of pixels so I wouldn't expect the PDF to report the same "size" as passed in the argument necessarily.
There was a problem hiding this comment.
I will also take a looksy on that issue, lets say for this and the above tomorrow. I am flying today.
|
cc @ayjayt |
|
Do we need to maintain backwards compatibility with older Kaleido versions here? |
There was a problem hiding this comment.
Are there percy tests that use kaleido to make sure that the actual images generated are correct?
25e7945 to
aca1620
Compare
Support both the Kaleido v0 and v1 APIs in Plotly.py.
fig.write_image(),fig.to_image(),pio.write_image(), andpio.to_image()to work with both Kaleido versionsengineargument toto_image()andwrite_image()kaleido.scope.*plotly.io.defaults.*. This can be used in place ofkaleido.scope.*and supports all the same settings:default_format,default_height,default_width,default_scale,mathjax, andtopojson.pio.defaultsis backwards-compatible with Kaleido v0.pio.write_images()function for generating multiple images at a timewrite_image()in a looppio.get_chrome()function which wrapskaleido.get_chrome_sync(), and add a CLI entry pointplotly_get_chromefor installing Chrome from the command linetests/test_optional/test_kaleido/test_kaleido.pytests to work with either Kaleido v0 or v1. They now also actually check that an image of the right format and dimensions has been generated, as opposed to using mocking to check that the right arguments are being passed to Kaleido.pio.write_images()TODO
engineargument towrite_image/to_imagemathjaxandtopojson