Fix image metadata

[flying-sheep]

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?

[welcome[bot]]

Thanks for submitting your first pull request! You are awesome! :hugs:
If you haven't done so already, check out EBP's Code of Conduct and our Contributing Guide, as this will greatly help the review process.
Welcome to the EBP community! :tada:

[ma-sadeghi]

Hi @flying-sheep, what needs to be done here? I might be able to look at it if it's reasonable effort.

[flying-sheep]

The changes to test output that I made (adding height=...) is exactly what I’m after. When e.g. matplotlib emits that metadata, it should end up becoming HTML attributes (not CSS*), which is what seems to be happening.

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 aspect-ratio from HTML width and height. So if you use HTML width and height, and also set max-width: 100%, you get the best of all worlds: An image that isn’t too big and also never squeezed or stretched.

[ma-sadeghi]

I'll look into it, but I'm no expert, so not sure if I succeed. For the time being, I switched to nbshpinx, because I really despise the large figures :grinning:

[agoose77]

@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 :)

[agoose77]

@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.

[flying-sheep]

In my view, the user can always tweak the image-producing code to change the size.

I agree. It should be between the theme and the user. MyST-nb shouldn’t be another factor that needs to be tweaked here.

When matplotlib says the metadata is {"width": "500px", "height": "300px"}, that directly translates to <img width="500px" height="300px">. Both things mean “instead of the image data’s intrinsic size, this is the size the image is intended to be displayed”. MyST-NB is just the broker passing this metadata on.

[flying-sheep]

Hm, looks like the hashes are no longer different. Now locally all tests pass, let’s see.

[agoose77]

The hashes had changed because, I presume, an update to the pandas LaTeX renderer produced a different image. That's a benign change, so I've manually checked in the changes.

[flying-sheep]

Well, as said: After merging in master, all is green locally, so maybe this is ready now!

[welcome[bot]]

Congrats on your first merged pull request in this project! :tada:
Thank you for contributing, we are very proud of you! :heart:

[flying-sheep]

woop! I have to say, your silly fish always cheers me up.

[ma-sadeghi]

@flying-sheep Thanks for getting this across the finish line!

[flying-sheep]

@OriolAbril I have a fix in https://github.com/executablebooks/MyST-NB/pull/599