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 support for math markdown #1220

Closed
wants to merge 4 commits into from
Closed

add support for math markdown #1220

wants to merge 4 commits into from

Conversation

0xVolodya
Copy link

Add support for math markdown

Fix iterative/gatsby-theme-iterative#73

Copy link
Contributor

@rogermparent rogermparent left a comment

Choose a reason for hiding this comment

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

Stripping spaces from the comments looks like harmless good practice to me, and the active code looks perfectly fine.
I need to see an article that has an example of this to truly know if it actually works all the way through, but if we want math syntax in remark this branch looks like a good solution.

The only concern I have is the possibility of client size increase in places that don't use math, but the MathJax wiki reaffirms that it shouldn't be an issue, so I don't see any problem in giving it a shot. I'll give it a few runs and maybe put together some test data, but this is already looking OK to me.

@@ -131,7 +132,7 @@
"gatsby-remark-responsive-iframe": "^2.3.1",
"gatsby-remark-smartypants": "^2.2.1",
"gatsby-source-filesystem": "^2.2.2",
"gatsby-transformer-remark": "^2.7.1",
"gatsby-transformer-remark": "^2.8.4",
Copy link
Member

Choose a reason for hiding this comment

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

@rogermparent it won't break the deployment right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm testing it right now, but I don't believe so. The only Gatsby plugins we need pinned to old versions currently are the ones specifically related to Sharp.
If it does break anything, I'll find out soon enough.

@rogermparent
Copy link
Contributor

I can confirm this PR builds well and doesn't break our caching, but I haven't seen an example of this functionality in use yet.

If we want to be as safe as possible, we can bring this into a branch of the official repo to let someone build the first in-content usage on top of it, then merge both this and the first math-using content in at the same time.

If simplicity is the priority, I see no harm in just merging this PR and letting the content that uses it come later.

Copy link
Contributor

@rogermparent rogermparent left a comment

Choose a reason for hiding this comment

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

It works and won't harm anything to merge now, but optionally we could merge later when the first usage of this occurs.
Either way, I think it deserves an approval.

@shcheklein
Copy link
Member

@rogermparent @nadalfederer we can test on this blog, I'll deploy this fork so that we can see it

@shcheklein
Copy link
Member

blog to test on: https://dvc.org/blog/a-public-reddit-dataset

@shcheklein shcheklein temporarily deployed to dvc-landing-1114-add-su-xfayuz May 3, 2020 21:03 Inactive
@rogermparent
Copy link
Contributor

rogermparent commented May 3, 2020

I'll try it out locally as well and report back, as well as push the change in here if it works.

@rogermparent
Copy link
Contributor

I got the formula looking good on that blog post! I'll clean it up and push the result in a bit.

@jorgeorpinel
Copy link
Contributor

Can you push it to this branch please? 🙂 Thanks Roger

@rogermparent
Copy link
Contributor

Ah, it looks like there's an issue with formula font sizes - specifically, they don't shrink on small screens if you want to make a large image-like formula like on the example article. Both block and inline versions suffer from this.
I'll still push the example up here, but that's something that needs figuring out before we start using these. Nothing that a little CSS can't solve, though!

@rogermparent rogermparent temporarily deployed to dvc-landing-1114-add-su-xfayuz May 3, 2020 21:49 Inactive
@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented May 3, 2020

Copy link
Member

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

PTAL - #1220 (comment) and other comments.

@rogermparent @jorgeorpinel thanks 🙏 for reviewing this!

@shcheklein shcheklein temporarily deployed to dvc-landing-1114-add-su-xfayuz May 4, 2020 18:06 Inactive
Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

LGTM now. Thanks @nadalfederer!

@rogermparent
Copy link
Contributor

Unfortunately, Mobile is still going to be an issue.

Screenshot from 2020-05-04 14-37-56

I apologize for trivializing the issue in my earlier comment, we'll need to have more complex CSS to get this to look okay in all the places the image did as images have the advantage of easy built-in scaling without worrying about line breaks. The solutions that come to my mind off the bat aren't quite simple ones, but I'll let everyone know if I can think of something minimally intrusive.

We may have to shelve this PR for a bit unless it's a priority.

Copy link
Contributor

@rogermparent rogermparent left a comment

Choose a reason for hiding this comment

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

Since the discovery from #1220 (comment), it turns out this isn't quite ready to bring into prod yet. We'll need some kind of solution to get these forumlae working on mobile, but it's a tricky situation where static line breaks are involved that the current image-based solution sidesteps entirely.

Even if we get a fully responsive set of sizes on this particular example, that may break on another formula of a different width. I think text-base formulae at a block level like in the article will require a unique font scale for every equation, and inline formulae will also break things unless we somehow allow them to reflow with text.

I think building on this plugin worth looking into, and the code written in the PR to add it is fine, but we need better responsive styling to make this sort of plugin a viable replacement for the current setup.

@shcheklein
Copy link
Member

@nadalfederer hey! any updates on this? do you need any help?

@fabiosantoscode
Copy link
Contributor

@rogermparent do you mean making formulae horizontally scrollable so mobile users can drag with their finger to see the rest? I read something from the mathjax blog which implies that responsiveness is kind of an open question for them.

@rogermparent
Copy link
Contributor

@rogermparent do you mean making formulae horizontally scrollable so mobile users can drag with their finger to see the rest? I read something from the mathjax blog which implies that responsiveness is kind of an open question for them.

That could be a solution, but it's not what I had in mind. I actually think that the best solution, if we really want to use a text-based solution for formulas, is to make a custom formula MD component that can accept a prop to set its own font size at different screen sizes.

This kind of solution is probably a bit much for a feature that isn't used all the time, so sticking with image formulas may be the best option for a while if not indefinitely.

If we implement a solution that makes formulas scrollable, we should keep in mind that that's only an imperfect solution to block type formulas. Not that we can easily use image formulas inline, but this text implementation can't boast the advantage of being usable inline while also having a much harder time being used as formulas currently are.

@fabiosantoscode
Copy link
Contributor

That could be a solution, but it's not what I had in mind. I actually think that the best solution, if we really want to use a text-based solution for formulas, is to make a custom formula MD component that can accept a prop to set its own font size at different screen sizes.

Are we able to drop react components into markdown, MDX style? I don't know. I was able to implement something like it when creating the clear: both element for floating images.

If we implement a solution that makes formulas scrollable, we should keep in mind that that's only an imperfect solution to block type formulas. Not that we can easily use image formulas inline, but this text implementation can't boast the advantage of being usable inline while also having a much harder time being used as formulas currently are.

I kind of see mathjax as the quintessential math language display library, since it's used in math.stackexchange.com and what-if.xkcd.com. So I suppose we'd better use it for maximum familiarity.

@rogermparent
Copy link
Contributor

rogermparent commented May 14, 2020

Are we able to drop react components into markdown, MDX style? I don't know. I was able to implement something like it when creating the clear: both element for floating images.

That's exactly what I had in mind. We can just change over the blog to use the rehype renderer the docs currently do, allowing us custom components in MD, even with Remark. The biggest issue I see so far is that doing it with Remark brings some weird limitations to the components that don't happen in MDX, and I wouldn't expect writers to like having to pick a few font sizes whenever they add a formula so it doesn't break on mobile. Maybe we could attempt an automatic solution that detects the formula's max character width and provides it font sizes accordingly, but that's quite a bit to do for text formulas.

I kind of see mathjax as the quintessential math language display library, since it's used in math.stackexchange.com and what-if.xkcd.com. So I suppose we'd better use it for maximum familiarity.

I understand going for it if we use these math formulas a lot, but adding them is going to be a bit of a larger overhaul than expected - to get Mathjax text formulas working while still having UX on par with the image solution currently used in the example blog post when screen sizes are regarded is actually quite a task in its current state.

It's also worth mentioning the first instance of mathjax on what-if.xkcd.com also breaks on mobile, but not as bad as ours because the wide single-line formula was added as its own block which let the thinner algebra block center itself properly on screens the other breaks on.

If it's important it's important, and the work is possible, but this isn't the quick fix it seemed to be in the first place where responsiveness is concerned.

A max-width: 100% and overflow: auto would probably get the scrolling effect that will let us use mathjax math blocks that won't totally break on mobile, but could possibly make for a clunkier experience for users than the image solution.

To be able to write mathjax and get images for the best of UX and DX, we could write our own plugin or modify gatsby-remark-mathjax to use Mathjax to output and insert SVG instead of HTML. This way, the output is guaranteed mobile friendly on all screens and can still be written in mathjax blocks. We won't even have to worry about image transforms since SVGs aren't transformed!

Wikipedia uses rendered SVG formulas for similar reasons that I just outlined. See this image from Wikipedia's page on the Pythagorean theorem:

https://wikimedia.org/api/rest_v1/media/math/render/svg/92333b53991e3ea02f5d6384bac4911ae3060a1e

Notice that, when opened, the page's SVG source is obviously MathJax

Seems Wikipedia uses a API to render math to SVGs, but since we have a build step we can render images then with no need for a separate server renderer.

@shcheklein
Copy link
Member

Closing this as stale. @rogermparent please feel free to move the great summary and other useful info from this attempt to the ticket.

@shcheklein shcheklein closed this May 20, 2020
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.

blog & docs: mathematical notation
5 participants