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
Support self-contained images in xaringan slides (close #3) #207
Conversation
…ding, issue warning if file does not exist
Is there anything else I should do for this? |
Nope. Sorry I haven't had a chance to review it yet... |
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.
Hi @srvanderplas! Could you rebase your PR?
Also please format the code in utils.R
and test-utils.R
to a width of 80.
Could you add an example to your first post showing the changed behavior? That would help a lot.
Afterwards I can have a detailed look at the code :)
…ding, issue warning if file does not exist
'tis done. Let me know if you need anything else.
…On Wed, Mar 4, 2020 at 1:19 AM Patrick Schratz ***@***.***> wrote:
***@***.**** commented on this pull request.
Hi @srvanderplas <https://github.com/srvanderplas>! Could you rebase your
PR?
Also please format the code in utils.R and test-utils.R to a width of 80.
Could you add an example to your first post showing the changed behavior?
That would help a lot.
Afterwards I can have a detailed look at the code :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#207?email_source=notifications&email_token=AAQHKFMQ4U6X7YOMKCA3IMTRFX6HZA5CNFSM4HGNKUVKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCX3ZW7I#pullrequestreview-368548733>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAQHKFNSNGMPHV3SZBWYFWDRFX6HZANCNFSM4HGNKUVA>
.
|
There seem to be quite some unrelated changes in this PR now. |
Right, something went wrong during the merge/rebase. @srvanderplas Could you please clean up and just keep the changes related to this PR? Atm there are many changes to files like NEWS.md or the GH issue template. |
|
I'm not religious about the width 80
…</img>; just <img />
…o find out images, base64 encode them, store the data in a JSON object, replace the original image URLs with special tokens that contain the original URLs; then in the browser, restore these tokens with the actual base64 data after the slides are rendered rendering slides should be fast because the Markdown content is still lightweight (contains no base64 data); Markdown content containing base64 data used to be the main bottleneck when rendering self-contained slides, because these huge base54 strings make it really slow for remark.js to render them
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've heavily tweaked this PR, and it should be ready to merge now. But I have just submitted a new version of xaringan to CRAN yesterday, so please do not merge this one until that version appears on CRAN (this PR will go to the next version).
I'm writing down the basic idea of the implementation here just in case anyone (including my future self) needs to understand it. Extract and (base64) encode images, and store the encoded data outside the Markdown text as a JS object. After remark.js has rendered the slides, insert the encoded images back to the slides. 1. Markdown images (could be
|
To contribute to this repo, please make sure to:
Thank you!
Demo: MWE with emoji icon
MWE in xaringan 0.15.1
MWE with self-contained image functionality
diff of HTML files (text file)
Rmd file for MWE