Skip to content
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

[ENH] Add initial svg handling #371

Closed

Conversation

JensHeinrich
Copy link

Removes a TODO
Adds a supported mime type for latex

This currently creates a svg-file from the inline svg component, which can be handled by other tools later

This basically fixes #370 as their the svg is now not handled as an text type anymore

Removes a TODO
Adds a supported mime type for latex

This currently creates a svg-file from the inline svg component, which can be handled by other tools later

This basically fixes executablebooks#370 as their the svg is now not handled as an text type anymore
@welcome
Copy link

welcome bot commented Oct 28, 2021

Thanks for submitting your first pull request! You are awesome! 🤗

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! 🎉

@JensHeinrich JensHeinrich changed the title Add initial svg handling [ENH] Add initial svg handling Oct 28, 2021
@codecov
Copy link

codecov bot commented Oct 28, 2021

Codecov Report

Merging #371 (300f481) into master (2baade0) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #371   +/-   ##
=======================================
  Coverage   87.35%   87.35%           
=======================================
  Files          12       12           
  Lines        1368     1368           
=======================================
  Hits         1195     1195           
  Misses        173      173           
Flag Coverage Δ
pytests 87.35% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
myst_nb/render_outputs.py 86.79% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2baade0...300f481. Read the comment docs.

@mmcky
Copy link
Member

mmcky commented Oct 28, 2021

thanks @JensHeinrich -- I'll leave this open for a day and will look to merge this tomorrow.

@mmcky mmcky added enhancement New feature or request latex labels Oct 28, 2021
@chrisjsewell
Copy link
Member

chrisjsewell commented Oct 28, 2021

This currently creates a svg-file from the inline svg component, which can be handled by other tools later

Heya, I don't think this is the answer, because it will break any build that does not have a specific extension installed to handle svg -> pdf conversions, and so should not be the default behaviour.

Note, you can already override this priority list in your conf.py, for non-default behaviours: https://myst-nb.readthedocs.io/en/latest/use/formatting_outputs.html#render-priority

Copy link
Member

@chrisjsewell chrisjsewell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comment

@mmcky
Copy link
Member

mmcky commented Oct 28, 2021

see comment

@chrisjsewell my plan is to review this with @AakashGfude tomorrow and follow a similar method that he has used to support png and gif images in sphinx-jupyterbook-latex but using the same converters that @JensHeinrich is using from underlying sphinx

https://github.com/executablebooks/sphinx-jupyterbook-latex/blob/cbbed833e8d4c211c0339d3464e623522186e471/sphinx_jupyterbook_latex/events.py#L106

@chrisjsewell
Copy link
Member

sphinx-jupyterbook-latex is still an extension external to myst-nb though, i.e. we could perhaps override the render-priority in the jupyter-book config, where we know it is installed, but it shouldn't be the default at the "myst-nb level"

@mmcky
Copy link
Member

mmcky commented Oct 28, 2021

@JensHeinrich do you use jupyter-book to compile your projects or do you just use myst-nb?

@JensHeinrich
Copy link
Author

@chrisjsewell still this way it is not handled as a string, which definitely is wrong (as described in #370)

So this way there is no "breaking" change but it is just ignored which seems to be cleaner to me

@JensHeinrich
Copy link
Author

I

@JensHeinrich do you use jupyter-book to compile your projects or do you just use myst-nb?

Jupyter-book

But as soon as the line is added, the data is available for sphinx to handle

@mmcky
Copy link
Member

mmcky commented Oct 28, 2021

But as soon as the line is added, the data is available for sphinx to handle

This is likely because jupyter-book loads sphinx-jupyterbook-latex which loads sphinx.ext.imgconverter by default.

@chrisjsewell
Copy link
Member

So this way there is no "breaking" change but it is just ignored which seems to be cleaner to me

What if the outputs in your notebook were only SVG and PNG; surely, in-lieu of any SVG converter, you would want the PNG to appear in your PDF build

@JensHeinrich
Copy link
Author

But as soon as the line is added, the data is available for sphinx to handle

This is likely because jupyter-book loads sphinx-jupyterbook-latex which loads sphinx.ext.imgconverter by default.

No, the sphinx-jupyterbook-latex needs to be handled additionally ATM

But there is a file which can be handled

@JensHeinrich
Copy link
Author

So this way there is no "breaking" change but it is just ignored which seems to be cleaner to me

What if the outputs in your notebook were only SVG and PNG; surely, in-lieu of any SVG converter, you would want the PNG to appear in your PDF build

Then probably the image/svg+xml should be handled later, but it just is no string

@AakashGfude
Copy link
Member

No, the sphinx-jupyterbook-latex needs to be handled additionally ATM

sphinx-jupyterbook-latex has been added as a default to jupyter-book from v0.10.2 onwards. So, SVG should be handled on the fly, regardless of this change.

@chrisjsewell
Copy link
Member

sphinx-jupyterbook-latex has been added as a default to jupyter-book from v0.10.2 onwards. So, SVG should be handled on the fly, regardless of this change.

jupyter-book is dependent on myst-nb, but myst-nb is in not dependent on jupyter-book (i.e. can and is used outside of jupyter-book)

Closing this as, since #380, this is easy to configure:

extensions =  ["sphinx.ext.imgconverter"]
nb_mime_priority_overrides = [
     # builder name, mime type, priority
	("latex", "image/svg+xml", 15)
]

this could be added to the documentation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request latex
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SVG outputs are rendered as strings in LaTeX
4 participants