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

Incorrect format for HTML tables #1954

Closed
cderv opened this issue Nov 18, 2020 · 6 comments · Fixed by yihui/knitr#1922
Closed

Incorrect format for HTML tables #1954

cderv opened this issue Nov 18, 2020 · 6 comments · Fixed by yihui/knitr#1922
Assignees

Comments

@cderv
Copy link
Collaborator

cderv commented Nov 18, 2020

Here is a minimal reprex

---
title: "table format"
output: html_document
---

```{r}
knitr::kable(iris, "html")
```

This is caused by the change in #1895 and the fact that kable html output does not include odd and even on rows.

Discovered by a user while loading kableExtra.

---
title: "table format"
output: html_document
---

```{r}
library(knitr)
library(kableExtra)
knitr::kable(iris)
```

kableExtra is modifying the format for knitr::kable that default to markdown pipe output. By auto formating to html, just loading the package modify the behavior of kable.

Two solutions:

For now the former seems better, as there is a JS solution for #1893 (see #1893 (comment))

cc @apreshill

@cderv cderv self-assigned this Nov 18, 2020
@cderv cderv added the bug an unexpected problem or unintended behavior label Nov 18, 2020
@cderv
Copy link
Collaborator Author

cderv commented Nov 24, 2020

In fact this is not a regression. knitr html table where not formatted as Pandoc Markdown table because the JS code does not target them.
With rmarkdown 2.4, before the aforementioned fix I thought was the cause.
image

That means HTML tables from knitr where not displayed the same as Pandoc markdown table and the fix in #1893 was not the cause.

This leave us with the question: Do we want knitr html table to be formatted the same as Pandoc mardown table ?

We may also add a test so that we are sure table existing framework are not impacted by our change... or is it something we should wait for revdep check to find out ?

@cderv cderv added Enhancement and removed bug an unexpected problem or unintended behavior labels Nov 24, 2020
@cderv
Copy link
Collaborator Author

cderv commented Nov 24, 2020

@yihui
Copy link
Member

yihui commented Nov 24, 2020

This leave us with the question: Do we want knitr html table to be formatted the same as Pandoc mardown table ?

That would be great. The only tricky thing is that the knitr html table doesn't have any special identifier or classes, so it may be hard to reliably detect tables generated by kable(, 'html'). Or perhaps we find out tables that do not have any attributes (id or class) at all and apply the table table-condensed classes? That might work.

If that still feels risky, users have to specify the class by themselves:

knitr::kable(, 'html', table.attr = 'class="table table-condensed"')

@cderv
Copy link
Collaborator Author

cderv commented Nov 24, 2020

Or perhaps we find out tables that do not have any attributes (id or class) at all and apply the table table-condensed classes? That might work.

I thought of that, but we would for example target flextable output which have no class or id on theader or tbody also. I tried it, and the output changed.

Without adding id or class to knitr output, I guess we need to rely on the table.attr as I think there is not really a way for the boostrap style table to target the right table without id or class.

The previous change we made (from tr.header to tr.odd) seems more target to pandoc tables.

@yihui
Copy link
Member

yihui commented Nov 24, 2020

Okay, let's just have users use table.attr if they really want the table to be styled with Bootstrap.

clrpackages pushed a commit to clearlinux-pkgs/R-knitr that referenced this issue Jan 28, 2021
Christophe Dervieux (20):
      fix #1893: make collapse = TRUE work when attr.source or classe.source is provided (#1902)
      fix rstudio/rmarkdown#1208: capture error in eng_sql() if the chunk option error = TRUE (#1910)
      fix #1912: stop early if RWordPress is not installed, and also warn against using this knit2wp() function (#1913)
      add lock bot (#1917)
      Run lock bot every week now
      Rename lock bot GHA workflow
      Use github actions instead of travis (#1920)
      Use linux on test-coverage workflow to avoid errors due to no binaries available in CRAN for macos
      do not check for newer source only-version
      forgot to remove travis
      do not error in R CMD CHECK if one suggested package is not installed
      Improve documentation following discussion in rstudio/rmarkdown#1954
      wrong place for _R_CHECK_FORCE_SUGGESTS_ env var.
      merge coverage in R-CMD-CHECK
      Workflow was renamed
      no more test coverage workflow
      close #1326: vectorize include_app() and include_url() (#1948)
      Add a fig.alt chunk option to provide alternative text different than fig.cap (#1900)
      the `table.attr` argument in `kable(.., format = 'html')` can take the value from the global option `knitr.table.html.attr` now (#1922)
      export and document pandoc_to() and pandoc_from() (#1951)

David C Hall (1):
      knit_exit() will exit the whole knitting process, even when called from a child document (#1810)

Luís Fonseca (1):
      close #1767: allows for a custom label for a table generated by kable() (#1890)

Sam Thompson (1):
      Extended combine_words with support for exclusion of the Oxford comma (#1946)

Yihui Xie (29):
      it seems I don't really need to import methods
      put methods back in Imports but don't import it in NAMESPACE
      just use xfun::read_utf8(), in case users have set options(encodng = ...), which I strongly recommend against
      install libharfbuzz-dev and libfribidi-dev for the new textshaping package (although I have no idea what it is), which is required by ragg, which is suggested by knitr
      TODO: remove these functions in knitr, and import them from xfun in the future
      remove some internal functions from knitr and import them from xfun 0.19 instead
      pandoc-citeproc has gone since Pandoc 2.11, so don't check for its existence any more
      also get rid of this hack for RStudio, and actually check for availability of Pandoc
      will asymptote work after installing dvisvgm?
      also install ghostscript
      fewer instances of ::: (highr:::group_src has been moved to xfun::split_source, and knit_code has been exported)
      suggest https://yihui.r-universe.dev instead for installing the dev version
      replace the download stats with CRAN badge instead
      generalize png_available() to dev_available() to test if a device is available
      close #1947: allow the graphical device to fall back to other operational devices when options(knitr.device.fallback = TRUE), which is the default for R CMD check
      move the code to select the graphical device out of save_plot(), and put it into dev_get() instead (renamed check_dev() to dev_get())
      use xfun helper functions
      63d153bf439dc7df73071bbf3d7d1ed0937e3eb7 changed the logic: we must make sure tikzDevice is loadable before we check other conditions
      make sure dpi is not NULL or NA
      initialize opts_current to the values of opts_chunk
      units of devices are always in inches, so we need to use smaller width and height to test if a device is available (close #1949)
      close #1925: delete the instructions on R CMD INSTALL/build; https://yihui.r-universe.dev is much more convenient and even has binaries
      fix #1945: instead of removing the YAML metadata from child document, replace it with blank lines, so the line numbers in error messages will not be affected
      close #1937: disable the table label with kable(label = NA)
      opts_current$get('label') is NULL when not running inside code chunks
      add an argument `fully` to knit_exit() to make it possible to exit the child document only (#1810)
      bookdown still uses :::is_abs_path()
      the bottomless hell of URL checking...
      CRAN release v1.31

atusy (1):
      Let collapse work even if class.* or attr.* are provided (closes #1906) (#1909)

christophe dervieux (5):
      Add a skip for NOT_CRAN
      Use full length commit sha to pin the action
      Change workflow name to try having the correct UI in Github Action pane
      Correct name so it avoids confusion in the UI
      Revert "Add a skip for NOT_CRAN"

dmurdoch (1):
      When counting plots, html_screenshots are missed. (#1908)
@github-actions
Copy link

This old thread has been automatically locked. If you think you have found something related to this, please open a new issue by following the issue guide (https://yihui.org/issue/), and link to this old issue if necessary.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
2 participants