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

put svg with xmlns in img src tag #1538

Merged
merged 11 commits into from
Mar 17, 2021

Conversation

jkrumbiegel
Copy link
Contributor

@jkrumbiegel jkrumbiegel commented Feb 26, 2021

This fixes #1537

A small test I made with Luxor, to show that inline svgs are correctly scaled down to fit the column width:

```@example
using Luxor
@svg begin
    background("black")
end 300 300
```

```@example
using Luxor
@svg begin
    background("red")
end 800 800
```

```@example
using Luxor
@svg begin
    background("blue")
end 1500 1500
```

before:

grafik

after:

grafik

src/Writers/HTMLWriter.jl Outdated Show resolved Hide resolved
src/Writers/HTMLWriter.jl Outdated Show resolved Hide resolved
@mortenpi
Copy link
Member

Could you also include a few additional test cases here: https://github.com/JuliaDocs/Documenter.jl/blob/master/test/examples/src/man/tutorial.md#including-images-with-mime

It would be good to have a small matrix of cases there, e.g. whether the <svg> has a viewBox or not, if it has width/height or not etc. Quickly reading through the article you linked, it sounded like even with img tags there might be edge cases where the SVG doesn't scale quite right?

@mortenpi mortenpi added Format: HTML Related to the default HTML output Type: Enhancement labels Feb 27, 2021
@jkrumbiegel
Copy link
Contributor Author

It would be good to have a small matrix of cases there

Would that be purely visual tests, checked manually? Because I don't think I can write a test that detects a browser's ability to show the embedded svg :)

@jkrumbiegel
Copy link
Contributor Author

jkrumbiegel commented Feb 28, 2021

I've made the following changes now:

SVG stays utf-8, with minimal changes to keep it from corrupting the surrounding html. I think that can also be nice if something doesn't look quite as expected, one can still inspect the SVG string manually in the browser, which doesn't work with base64. And base64 seems to inflate the size anyway.

I splice in the xmlns tag if it's not there, I couldn't find reasons why I shouldn't.

I've added a couple test cases on the page you linked.

@jkrumbiegel
Copy link
Contributor Author

I'd like to bump this as Makie's documentation could really use a simpler way to return svg's from example blocks

Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

Sorry @jkrumbiegel, this dropped off my radar. Let's get this wrapped up.

src/Writers/HTMLWriter.jl Outdated Show resolved Hide resolved
@mortenpi mortenpi added this to the 0.26.4 milestone Mar 14, 2021
Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

LGTM. Do you mind using Documente#master for Makie for a while though? I would like to leave this change for 0.27, but I don't want to rush that. This can easily be done if you commit the docs/Manifest.toml.

src/Writers/HTMLWriter.jl Outdated Show resolved Hide resolved
@mortenpi mortenpi modified the milestones: 0.26.4, 0.27.0 Mar 14, 2021
Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
@jkrumbiegel
Copy link
Contributor Author

No I don't mind using master, thanks for helping to push this over the finish line.

@devmotion
Copy link
Contributor

I experienced the same problem with Jupyter notebooks, so maybe it would be useful to add the same fix to IJulia?

xref: https://discourse.julialang.org/t/cairomakie-and-fontconfig/55931/17?u=devmotion

@mortenpi
Copy link
Member

Thanks for iterating on this @jkrumbiegel!

@mortenpi mortenpi merged commit a7df180 into JuliaDocs:master Mar 17, 2021
@cormullion
Copy link
Contributor

Did this PR fix the "text in SVG images gets corrupted" issue? Still seeing this problem...

@jkrumbiegel
Copy link
Contributor Author

Yes, but it's only on master for now

@cormullion
Copy link
Contributor

OK, thanks (and for the fix!). I'll put my doc updates aside for a while...

@mortenpi
Copy link
Member

I'll put my doc updates aside for a while...

If you have some docs that need this now, I would suggest just using #master for now by committing a docs/Manifest.toml file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Format: HTML Related to the default HTML output Type: Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use URI-encoding for SVG images coming from at-example blocks
4 participants