ENH: pass-through image metadata #588
Conversation
|
Thanks for submitting your first pull request! You are awesome! 🤗 |
for more information, see https://pre-commit.ci
|
Hi @flying-sheep, what needs to be done here? I might be able to look at it if it's reasonable effort. |
|
The changes to test output that I made (adding What needs to be done: I don’t understand the test failure: what do these hashes mean and why do my changes cause them to change? If it’s a quick fix, I’m happy to do it, but as said, I only care about the result, so if you want to take this up and update this PR until tests pass, feel free. *why not CSS? Because browsers derive |
|
I'll look into it, but I'm no expert, so not sure if I succeed. For the time being, I switched to |
|
@flying-sheep thank you for this PR! It looks like a good change. In future, we might want to allow the user to override these settings, though I'm not 100% sure -- In my view, the user can always tweak the image-producing code to change the size. If we want this to be overridable, we can do it at a later date with a new config variable that can be set at cell, notebook, or project level. I'll take over this PR to get it across the finish line :) |
|
@ma-sadeghi I just saw your reply! We are always keen to have new contributors, are you still interested in pushing this forward given my reply above? I'm happy to help where necessary. |
I agree. It should be between the theme and the user. When matplotlib says the metadata is |
|
Hm, looks like the hashes are no longer different. Now locally all tests pass, let’s see. |
|
The hashes had changed because, I presume, an update to the |
|
Well, as said: After merging in |
|
Congrats on your first merged pull request in this project! 🎉 |
|
woop! I have to say, your silly fish always cheers me up. |
|
@flying-sheep Thanks for getting this across the finish line! |
There was a problem hiding this comment.
Something is not working right after this PR, and I think the updated tests are testing for the failure.
After this, if one image has height/width attributes, all images are affected by that, not only the image with those attributes. I have commented below and added screenshots of the outputs in the complex_outputs.ipynb notebook used for testing, however, the height=400 is not only applied to all images of the complex_outputs notebook, but to all images, also all images in the matplotlib.ipynb notebook.
I have used a very minimal configuration and with the alabaster theme to try and discard theme issues, but it is not a theme specific issue
as I have seen it with multiple themes.
MWE repo: https://github.com/OriolAbril/sphinx-playground/tree/3eccd4b738070c15016c29f3640f0b82ad2cab74
| @@ -109,7 +109,7 @@ | |||
| <container classes="cell_output" nb_element="cell_code_output"> | |||
| <container nb_element="mime_bundle"> | |||
| <container mime_type="image/jpeg"> | |||
| <image candidates="{'*': '_build/jupyter_execute/a4c9580c74dacf6f3316a3bd2e2a347933aa4463834dcf1bb8f20b4fcb476ae1.jpg'}" uri="_build/jupyter_execute/a4c9580c74dacf6f3316a3bd2e2a347933aa4463834dcf1bb8f20b4fcb476ae1.jpg"> | |||
| <image candidates="{'*': '_build/jupyter_execute/a4c9580c74dacf6f3316a3bd2e2a347933aa4463834dcf1bb8f20b4fcb476ae1.jpg'}" height="400" uri="_build/jupyter_execute/a4c9580c74dacf6f3316a3bd2e2a347933aa4463834dcf1bb8f20b4fcb476ae1.jpg"> | |||
There was a problem hiding this comment.
I understand this image gets the height=400 metadata. However, it seems to end up being css instead of html attribute.
From the description and the xml here I expected to get <img ... height="400px"> instead of the <img ... style="height: 400px;"> that shows up.
There was a problem hiding this comment.
Yeah, seems like this one is the only one that should get that, weird that the others do, I’ll check that out.
Regarding CSS: That’s another issue, but it should definitely be fixed, I agree!
|
@OriolAbril I have a fix in #599 |
Fixes #522
I’m not super interested in getting this to the finish line, so please feel free to take it over and change it in any way you see fit.
The important part is that #522 gets fixed.
The only question is precedence: Will
.get_cell_level_config()return a width and height if e.g. only a global or notebook-wide default width/height is specified? Which should override which?