Skip to content

Add equation support (KaTeX)#5

Open
zohaad wants to merge 2 commits into
tonarino:mainfrom
zohaad:main
Open

Add equation support (KaTeX)#5
zohaad wants to merge 2 commits into
tonarino:mainfrom
zohaad:main

Conversation

@zohaad

@zohaad zohaad commented Apr 27, 2022

Copy link
Copy Markdown

I got equation support working:
image

Feel free to change any/all code, I'm new to next.js/JSX etc. so best practices are likely not followed.

@zohaad zohaad mentioned this pull request Apr 27, 2022
9 tasks
@bschwind

Copy link
Copy Markdown
Member

Hey, this is really cool! I'll need a bit of time to properly review this. I'm not sure of others' opinions on this, but for the katex CSS I think I'd prefer it added directly to this repo with a version in the file name. This way the statically-generated site doesn't need to depend on cdn.jsdelivr.net or that particular version of katex being there.

I'll try to get a more thorough review later.

@zohaad zohaad marked this pull request as draft April 27, 2022 14:12
@zohaad zohaad marked this pull request as ready for review April 27, 2022 14:31

@bschwind bschwind left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looking good so far, I left a few comments.

I know it's somewhat annoying, but could you place all the katex fonts into a public/fonts/katex directory? It's quite a lot of files to mix in with the existing fonts (which should probably also be organized at some point...)

Comment thread src/pages/[slug].tsx
break
case 'equation':
elements.push(
<div key={block.id} style={{ display: 'flex', justifyContent: 'center', alignItems: 'center' }}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you create a .equation class inside of blog.module.css which contains these CSS declarations?

Comment thread src/pages/[slug].tsx
<div key={block.id} style={{ display: 'flex', justifyContent: 'center', alignItems: 'center' }}
dangerouslySetInnerHTML={{
__html: katex.renderToString(block.equation.expression, {
throwOnError: false

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Did you find that this code would often throw exceptions? I'm not sure if it's better to create an error on invalid equation content, or to ignore it and keep going.

Comment thread yarn.lock
resolved "https://registry.yarnpkg.com/katex/-/katex-0.15.3.tgz#08781a7ed26800b20380d959d1ffcd62bca0ec14"
integrity sha512-Al6V7RJsmjklT9QItyHWGaQCt+NYTle1bZwB1e9MR/tLoIT1MXaHy9UpfGSB7eaqDgjjqqRxQOaQGrALCrEyBQ==
dependencies:
commander "^8.0.0"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd prefer not to downgrade a dependency just because a new dependency needs an older version. Is this absolutely required for katex to function? It seems a bit strange for a CLI argument parser library to be required when we're not using the CLI explicitly.

@bschwind

Copy link
Copy Markdown
Member

Also if it's not too much trouble, it might be nice to also support the inline_equation block type which notion also has.

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