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

Update diagrams #914

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

predictiple
Copy link
Contributor

@predictiple predictiple commented Sep 20, 2024

  • Updated fonts (got rid of Schoolbell because it was too scrawly)
  • Made some small diagram improvements.
  • Updated Excalidraw file format via excalidraw.com
  • Exported diagrams from excalidraw.com in SVG format
  • Manually replaced the SVG font defs, substituting with our hosted embedded fonts.
  • Updated the markdown file refs.

@predictiple predictiple marked this pull request as draft September 20, 2024 08:24
@predictiple
Copy link
Contributor Author

Apparently the fonts won't render unless they are embedded.
https://dzx.fr/blog/svg-font-embedding/

SVG is complicated
https://css-tricks.com/using-custom-fonts-with-svg-in-an-image-tag/

@predictiple
Copy link
Contributor Author

They also don't render in notebooks

Screenshot from 2024-09-20 10-49-00

@scudette
Copy link
Collaborator

That's because we don't serve svg files with the correct mime types

@predictiple
Copy link
Contributor Author

Embedding a minimal subset of the fonts adds between ~60kB to the SVGs compared to Excalidraw's embedded fonts. This is because they subset each of them to the glyphs actually used before b64 encoding.

@scudette
Copy link
Collaborator

I think that's acceptable no?

Can we just inline the SVG? it seems preferrable anyway - we can get hugo to just expand them inline.

@predictiple predictiple marked this pull request as ready for review September 20, 2024 12:43
@predictiple
Copy link
Contributor Author

Yes I think it's fine.

There are optimizations that can be done like this for example but for now I'm happy if it just works.

I don't know if inlining is preferable. This article suggest that it's not:
https://css-tricks.com/using-custom-fonts-with-svg-in-an-image-tag/#aa-problems-with-inline-svg

@scudette
Copy link
Collaborator

Right this is because inlining breaks caching and compression on dynamic sites. Since hugo is a static site it actually does not - the entire html page is cached and compressed because it is static

@predictiple
Copy link
Contributor Author

Something else to consider: inlining and moving the font defs to the web page will make them non-portable. They would have to be saved into another format to use in training and presentations slides, and notebooks.

Screenshot from 2024-09-21 11-17-33

@predictiple
Copy link
Contributor Author

I'm impressed that the inline SVG works. But I'm still divided over whether it's the right way to go because - aside from the portability issue mentioned above - ...

Markdown images are more concise and syntax highlighted
Screenshot from 2024-09-21 22-36-43

Right clicking on an inline SVG and choosing "Open image in a new tab" only opens the PNG layer. Likewise choosing "Save image as..." gives just the PNG component and the filename "download.png".

@scudette
Copy link
Collaborator

That's easy to fix with a presubmit

Realistically there are a number of cases where we can't use the markdown figures because they are too limited. Often we need to use an IMG tag directly because markdown does not support sizes or classes.

That's not a big problem. We can convert all markdown to a figure shortcode with a script.

With a shortcut we can actually render a better output. I think now there is a hacky script to convert IMG tags to Featherlight tags so it would be good to clean that up as well

@predictiple
Copy link
Contributor Author

We almost do this already - convert markdown images to figures:
https://github.com/Velocidex/velociraptor-docs/blob/master/layouts/_default/_markup/render-image.html

If we could extend that the the SVG inlining and thus retain markdown image syntax that would be better. The less shortcode we have in the content the easier it will be to migrate away from Hugo in future, or at least test other options. The cases where markdown is insufficient should really be rare.

We also need to keep in mind that Featherlight is basically specific to the Learn theme. Even the ReLearn theme has moved away from using it. So even switching to a better theme is probably going to necessitate independence from it.

The SVG inlining is really a size vs portability trade-off. The above right-click issues also being in the portability category. I lean towards the portability being more important.

@scudette
Copy link
Collaborator

Right click to save seems like such a random requirement. Who would really do that anyway? It can be easily solved by adding a download link to the figure if we really cared.

@predictiple
Copy link
Contributor Author

I think that "work like a normal website" is a reasonable requirement. Our choice to use SVG images shouldn't affect users. They shouldn't have to even think about it.

If a user opens an image in a new tab so that they can refer to it while they continue reading down the page, they'll be surprised that the annotations disappear from the image because that half of the image is part of the page. If they want to share a link to an image with their colleagues they get a data URI (minus annotations). We understand the reasons but for a typical user that's weird.

Behaving to expectations is part of the user experience. We should try to not be weird. Our goal is just to do what other (good) documentation sites do.

@scudette
Copy link
Collaborator

I forgot about the default image render hook https://gohugo.io/render-hooks/images/

There are some other interesting render hooks we might want to use as well.

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.

2 participants