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

Add HTML support for showing images #50

Merged
merged 5 commits into from
May 1, 2022
Merged

Conversation

johnnychen94
Copy link
Member

@johnnychen94 johnnychen94 commented May 1, 2022

Can't push to #49 so I open a new PR here. Changes on top of #49:

  • set default format PNG, because PNG is lossless and supports transparency.
  • reuse the show_element codes
  • "fixes" the test on 1.0: this is because ImageIO is only available on Julia >= 1.3 and thus reference results may change.

closes #48
closes #49

cc: @lorenzoh

@codecov
Copy link

codecov bot commented May 1, 2022

Codecov Report

Merging #50 (6b8583d) into master (324e8a4) will increase coverage by 0.09%.
The diff coverage is 93.75%.

@@            Coverage Diff             @@
##           master      #50      +/-   ##
==========================================
+ Coverage   87.89%   87.98%   +0.09%     
==========================================
  Files           6        6              
  Lines         223      233      +10     
==========================================
+ Hits          196      205       +9     
- Misses         27       28       +1     
Impacted Files Coverage Δ
src/ImageShow.jl 0.00% <ø> (ø)
src/showmime.jl 95.55% <93.75%> (-0.70%) ⬇️

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 324e8a4...6b8583d. Read the comment docs.

@lorenzoh
Copy link
Contributor

lorenzoh commented May 1, 2022

Looks good to me 👍

@johnnychen94 johnnychen94 merged commit 417bcb2 into master May 1, 2022
@johnnychen94 johnnychen94 deleted the lorenzoh/master branch May 1, 2022 15:23
@johnnychen94
Copy link
Member Author

johnnychen94 commented May 1, 2022

This might break some people's workflow, I'm not sure. But let's check it out in the wild JuliaRegistries/General#59456

@fonsp
Copy link
Contributor

fonsp commented May 11, 2022

This does not work well in Pluto notebooks, where suddenly our specialized PNG display (with native performance and nice scaling and reduced layout shifts) is replaced by a blurry small inline image 😭

image

We use Images.jl extensively in https://computationalthinking.mit.edu/ , where the automatic scaling causes confusingly small and blurry images. I now have to add an override to disable this PR:

Base.showable(::MIME"text/html", ::AbstractMatrix{<:Colorant}) = false

@lorenzoh can I work with you to add the PNG->HTML to Pollen.jl directly, instead of to this package? Or maybe we can hide this change behind a flag?

@fonsp
Copy link
Contributor

fonsp commented May 11, 2022

Another example from the 3b1b lecture:

Before

Schermopname.2022-05-11.om.20.36.35.mov

After this PR

The image is too small, and you often get vertical layout shifts when the <img> element is replaced by the new HTML output:

Schermopname.2022-05-11.om.20.35.14.mov
Schermopname.2022-05-11.om.20.34.27.mov

@johnnychen94
Copy link
Member Author

Oh, this is so unfortunate! I knew there will be some conflict if the same HTML output feature is defined in downstream packages but didn't expect it to be undesired as this (especially the vertical layout shifts).

This was referenced May 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Output embeddable HTML directly
3 participants