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

Convert math in README #2263

Merged
merged 10 commits into from
May 14, 2024
Merged

Convert math in README #2263

merged 10 commits into from
May 14, 2024

Conversation

maelle
Copy link
Collaborator

@maelle maelle commented Jan 6, 2023

Fix #2261
See also #2262 (would better be done in a separate PR)

I got a bit stuck trying to test this as it was hard for me to catch the Pandoc error in a test (it appeared in the log but I could not catch it 😅) hence the new Pandoc option for making warnings failures.

In any case the whole point is adding --mathjax to the Pandoc call.

@maelle maelle changed the title snapshot math in README Jan 6, 2023
@cthombor
Copy link

cthombor commented Jan 17, 2023

Hi Maelle, FYI I'm still unable to render $\sqrt{x}$ in https://cthombor.github.io/SafeVote/; but I do have another clue which might help localise the defect... or perhaps just indicates that a repair is still in the process of being deployed!

Using devtools::check_rhub() today, I got a couple of notes:

❯ checking HTML version of manual ... [11s] NOTE
Skipping checking math rendering: package 'V8' unavailable

❯ checking for detritus in the temp directory ... NOTE
Found the following files/directories:
'lastMiKTeXException'

I don't find a copy of lastMiKTeXException in my check_rub() results

@maelle
Copy link
Collaborator Author

maelle commented Jan 20, 2023

the fix is in this branch only for now :-)

@salim-b
Copy link
Collaborator

salim-b commented Jan 25, 2023

@maelle Pandoc's --mathjax option optionally allows to specify a URL pointing to the MathJax assets to be used. If none is provided (like in your current PR),

a link to the Cloudflare CDN will be inserted.

This is kinda detrimental to the goal of #2249. 😐

Furthermore, I don't quite understand why the README is treated differently than articles/vignettes... pkgdown already includes MathJax assets, so it should really be possible to use the same approach to build both the README and articles/vignettes (where inline math is properly rendered), no?

@maelle
Copy link
Collaborator Author

maelle commented Jan 26, 2023

@maelle Pandoc's --mathjax option optionally allows to specify a URL pointing to the MathJax assets to be used. If none is provided (like in your current PR),

This is kinda detrimental to the goal of #2249. neutral_face

Good to know, thank you! I guess that if we keep this option, we'll need to add the URL then.

Furthermore, I don't quite understand why the README is treated differently than articles/vignettes... pkgdown already includes MathJax assets, so it should really be possible to use the same approach to build both the README and articles/vignettes (where inline math is properly rendered), no?

The big difference is that vignettes/articles are knit whereas the README is not. Now, it does not mean approaches can't be aligned somehow. 😅

@hadley
Copy link
Member

hadley commented Apr 19, 2024

Probably worth digging into how RMarkdown sets the --mathjax argument so we can make sure to align.

Copy link

github-actions bot commented May 13, 2024

@hadley
Copy link
Member

hadley commented May 13, 2024

@maelle can you please take another look at this? I just tweaked the way that you test the code, adding some future looking support to make it easier to modify existing pkgdown test sites.

@hadley hadley changed the title math in README Convert math in README May 13, 2024
Copy link
Collaborator Author

@maelle maelle left a comment

Choose a reason for hiding this comment

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

🙏


if (clone) {
if (is.null(path)) {
cli::cli_abort("Can only clone when {.arg path} is set.")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

why an else after an error?

Copy link
Member

Choose a reason for hiding this comment

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

I think this if makes the logic most clear, but it doesn't matter.

@@ -40,6 +42,23 @@ local_pkgdown_site <- function(path = NULL, meta = NULL, env = parent.frame()) {
desc$write(file = path(path, "DESCRIPTION"))

file_create(path(path, "_pkgdown.yml"))
}

if (clone) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the argument name made me think a local Git clone would be involved

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm, yeah, copy would be a better name. But I'll probably end up refactoring this into a couple of different functions, so I'm going to leave it for now.

@hadley hadley merged commit ac1a5ce into main May 14, 2024
13 checks passed
@hadley hadley deleted the readme branch May 14, 2024 16:48
SebKrantz pushed a commit to SebKrantz/pkgdown that referenced this pull request Jun 1, 2024
Fixes r-lib#2261

Co-authored-by: Hadley Wickham <h.wickham@gmail.com>
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.

[WARNING] Could not convert TeX math z = s\sqrt{n}, rendering as TeX
4 participants