Repository navigation
Conversation
Synchronise branch with develop
| #' | ||
| #' shinyApp(ui, server) | ||
| #' } | ||
| ro_reactable_css <- function(sorting = TRUE) { |
There was a problem hiding this comment.
I would put this default as sorting = FALSE, so it is aligned with the ro_reactable_theme default (and also with Laurens-table opinion).
| local_mocked_bindings( | ||
| ro_check_if_font_available = function(target_font_family) "Verdana" | ||
| ) | ||
| css <- ro_dt_theme()$children[[1]]$children[[1]] |
There was a problem hiding this comment.
The test is using the ro_dt_theme instead of the ro_reactable_theme
|
|
||
| test_that("ro_reactable_theme errors for an unavailable custom font", { | ||
| expect_error( | ||
| ro_dt_theme(base_family = "Not an installed font"), |
There was a problem hiding this comment.
The test is using the ro_dt_theme instead of the ro_reactable_theme
| #' shinyApp(ui, server) | ||
| #' } | ||
| ro_reactable_css <- function(sorting = TRUE) { | ||
| arrow_css <- if (sorting) { |
There was a problem hiding this comment.
Not something for now, but maybe for the future(?):
Do you think its possible to have one css function for the arrow-theming, which then can be applied to both reactable and DT? In that way, it would be possible to keep the arrow-theming in one place. (Or maybe even more broadly, for the table theming in general).
@mplasmans @hypebright What do you think?
There was a problem hiding this comment.
Yeah that sounds like a good idea @tkranenburgrivm! Keeps it DRY. We would also need to make sure that we avoid hard-coding any colors that are organisation-dependent. E.g. #44688f, #f2f2f2 and #007bff instead of coming from ro_color()? Or is that overkill and not relevant for an arrow @mplasmans 🤓?
| ) { | ||
| check_string(base_family) | ||
|
|
||
| base_family <- ROvis.utils::ro_check_if_font_available( |
There was a problem hiding this comment.
Maybe something minor (I'm not sure if it has impact on anything): The gt and DT theme call it without the ROvis.utils::. It's already loaded with the NAMESPACE importFrom
There was a problem hiding this comment.
It's already imported here:
ROvis.table/R/ROvis.table-package.R
Line 13 in d286f68
importFrom here or call ROvis.utils::
|
@mplasmans Hi Marjanne, super cool you are adding the reactable to the package! I hope this will help a lot with updating the VZinfo dashboards in the future. I left some comments on the code, and I will leave some future suggestions here. But I would say the code itself looks good, so good by me to merge:)
|
|
Nice @mplasmans!! I took a little peek at it even though @tkranenburgrivm is the reviewer because I was tagged anyway 😄. Yes to the vignette, that would be great (can be separate issue indeed), also to include it in the README. Make sure people know it's there. I think the other points also make sense and deserve a spot in the board. The theme can have a test in this PR (this is nice for test coverage percentage 👼 ). |
|
By the way, this closes #10 right? |
This branch has not been deployed
3 participants
Reactable tabel toegevoegd in dezelfde stijl als de DT table.