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

Fix issue with markdown and Quarto #1958

Merged
merged 3 commits into from
Feb 24, 2025
Merged

Conversation

olivroy
Copy link
Collaborator

@olivroy olivroy commented Feb 21, 2025

Summary

Basically this reverts part of #1860.

As suggested in quarto-dev/quarto-cli#11932 (comment), we encode to base64 the unprocessed text.

It seems to do the trick and does not seem to introduce regressions (in particular the example in #1773 renders correctly)

For the review, it would be best to investigate this 60f19ad instead of the full diff because I added tests in the first commit.

@cderv , do you think this is the right approach?

Related GitHub Issues and PRs

Fixes #1957

Fixes quarto-dev/quarto-cli#11932

Fixes quarto-dev/quarto-cli#11610

investigation

Here is an issue I found after this change in typst:

  • Not getting anything with html-table-processing: none. So, far I haven't seen issues on html output
---
title: test
format: typst
execute:
  echo: false
---

```{r}
devtools::load_all()
```

```{r}
#| html-table-processing: none
dplyr::tibble(
  char = LETTERS[1:2],
  num = c(1.2, -33.52),
  text = c("- x", "- y"),
  text2 = "[gog](https://google.com)"
) |>
  gt() |>
  fmt_markdown() %>% 
  gt::tab_source_note(md("[google](https://google.com)"))

```

With the following qmd

---
title: test
format: html
execute:
  echo: false
---

```{r}
devtools::load_all(
)

```

```{r}
dplyr::tibble(
  char = LETTERS[1:2],
  num = c(1.2, -33.52),
  text = c("- x", "- y"),
  text2 = "[gog](https://google.com)"
) |>
  gt() |>
  fmt_markdown() %>% 
  gt::tab_source_note(md("[google](https://google.com)"))

```

```{r}
exibble %>%
    dplyr::select(num, char, fctr) %>%
    dplyr::mutate(x = "- 1") %>%
    dplyr::slice_head(n  = 5) %>%
    gt() %>%
    fmt_markdown(num) %>%
    tab_footnote(
      md("Problem because num row 1 is `fmt_markdown()` + also the footnote is wrapped in md."),
      locations = cells_body("num", 1)
    ) %>%
    tab_footnote(
      "A problem because fctr is labelled with md",
      locations = cells_column_labels("fctr")
    ) %>%
    tab_footnote(
      "Not a problem",
      locations = cells_column_labels("char")
    ) %>%
    cols_label(fctr = md("Factor")) %>%
    tab_header(md("[gog](https://google.com)")) %>%
    tab_spanner(md("problem"), c(2, 3))
```

gt 0.11.1:

Image

This PR:

Image

Checklist

@olivroy

This comment was marked as duplicate.

Copy link
Member

@rich-iannone rich-iannone left a comment

Choose a reason for hiding this comment

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

LGTM!

@rich-iannone rich-iannone requested a review from cderv February 21, 2025 20:32
@rich-iannone
Copy link
Member

@olivroy this looks really good to me! We'll hold off on merging until @cderv has a chance to look this over.

Copy link
Collaborator

@cderv cderv left a comment

Choose a reason for hiding this comment

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

I see in #1860 that <span> are used instead of <div> (b0a8c76 (#1860)). I suggested this idea (#1605 (comment)) but I remember a later discussion with @rich-iannone about this being a bad idea because of the content that could be inside the cell of gt that could sometime not compatible with span. 🤔 I don't have example in mind though...

If you think this is all good, then let's merge once the news item is tweaked.

I'll track quarto-cli issue related to this gt change to see if this fix others.

thanks !

NEWS.md Outdated
@@ -1,5 +1,8 @@
# gt (development version)

* Fixed an issue where `fmt_markdown()` could create strange output in Quarto (html and Typst formats) (@olivroy, #1957, [quarto-dev/quarto-cli#11932](https://github.com/quarto-dev/quarto-cli/issues/11932)).
This means that using `#| html-table-processing: none` may no longer work as expected in some cases.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is puzzling to me. I would like to understand why.

#| html-table-processing: none is just a way to tell Quarto to not process the HTML table. It should not have impact on the output.

If this is a matter of gt that needs to know about it, then we can make sure this is communicated correctly to engine. But this is not possible on all cases considering different ways to set it. https://quarto.org/docs/authoring/tables.html#disabling-quarto-table-processing

Anyway, I don't think this should have an impact, but I can see that the HTML will be different in Quarto context.

gt also has an option that is supposed to pass this information to quarto
https://gt.rstudio.com/reference/tab_options.html?q=tab_options#arg-quarto-use-bootstrap-quarto-disable-processing

It would be good to be sure this still works as expected. And maybe this should be prioritize way to set it for gt 🤔

However, if you are reffering to using html-table-processing: none on gt table to render to format: typst, then no output is expected because

  • gt produces HTML raw content and not typst raw content
  • html-table-processing: none means explicit opt-out of Quarto parsing HTML table to render to any format table using Pandoc writers for Table element.

If this why you put the comment in news, then I think it can be removed. This is all working as expected

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

However, if you are reffering to using html-table-processing: none on gt table to render to format: typst, then no output is expected because

This is what I mean indeed!

Ok, I think this should be documented in the Quarto documentation in this case, this seems a bit counter-intuitive to me.

Maybe in https://quarto.org/docs/authoring/tables.html

I will remove this line from news

Copy link
Collaborator

Choose a reason for hiding this comment

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

We document HTML table parsing in there: https://quarto.org/docs/authoring/tables.html#html-tables

As a result, you can use HTML table syntax in your documents and it will be converted to Markdown syntax for all formats.

And then we document disabling: https://quarto.org/docs/authoring/tables.html#disabling-quarto-table-processing

When you disable Quarto’s HTML table processing, tables are not translated to Markdown, will not be rendered to other output formats

How would you make this clearer ? or How would you present it differentlly ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess it is implied. I just guess it would be good to say something specific about format: typst in the quarto Disabling Quarto Table Processing.

Maybe

---
format:
  typst:
    html-table-processing: none
---

For the typst format, you will not see tables if you use this?

But I am okay either way.

@cderv
Copy link
Collaborator

cderv commented Feb 24, 2025

I'll track quarto-cli issue related to this gt change to see if this fix others.

This PR will also solve this issue

  • rendering a gt table with typst in R quarto quarto-dev/quarto-cli#11610
    Using markdown for newline works with typst format
     ---
     title: "Untitled"
     format: typst
     keep-md: true
     ---
     
     ```{r}
     #| echo: false
     #| message: false
     library(tidyverse)
     # devtools::load_all()
     library(gt)
     
     head(mtcars) %>% 
       select(1:4) %>% 
       gt() %>% 
       tab_source_note(md("*Bla*. hello i'm a footnote thats  
     too long for this table."))
     ```
    image

@cderv cderv linked an issue Feb 24, 2025 that may be closed by this pull request
@olivroy olivroy requested a review from cderv February 24, 2025 14:18
@olivroy
Copy link
Collaborator Author

olivroy commented Feb 24, 2025

Thanks for the explanations!

One last thing: if we revert back to using div recreates #1773. So, until we have a clear use case of going back to div, I think we should stick to span for now.

@rich-iannone
Copy link
Member

I think we ought to keep using span because you did solve that important issue in #1773 and we can always explore using a better div implementation (if it exists) in the future.

@rich-iannone
Copy link
Member

@olivroy thanks for doing this important work! I will merge the PR now.

@rich-iannone rich-iannone merged commit 37a25ab into rstudio:master Feb 24, 2025
12 checks passed
@rich-iannone
Copy link
Member

@cderv thanks for your review of this! #1958 is merged now so any downstream issues that are fixed by this could probably be closed. I also intend to submit gt to CRAN very soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants