Research switch to bootstrap 4

gogonzo commented 2 years ago

Please see @Polkas comment https://github.com/insightsengineering/teal/issues/617

Scope:

include BS4 source into teal or/and teal.widgets
check what doesn't work and need fixes
create the issue for each "BUG"

Polkas commented 2 years ago

Objective SUpport BS3 , BS4 and BS5 at the same time in the project.

[x] error from shinyWidgets::switchInput https://github.com/dreamRs/shinyWidgets/issues/528 (FIXED ON shinyWidgets main branch)
[x] modules layout (encoding.main plot/ filterpanel) under different bs versions, broken for higher versions (FIXED mainly by removing unnecessary div's)
[x] main fluidPage in the teal to inject the bootstrap theme; done with the additional option like https://github.com/insightsengineering/teal/blob/main/vignettes/teal-options.Rmd + add to vignette (DONE partly , vignette needed)
[ ] check non teal::init related vignette examples, style could be broken
[x] write the vignette how to specify the new bstheme (with bslib::bs_theme_preview() and bslib::run_with_themer) and then inject into the teal app. adding custom css to the teal app https://rstudio.github.io/bslib/reference/bs_bundle.html add sth about custom UI. (IN PROGRESS)
[x] tag hooks to achieve a dynamic render under different versions of bootstrap; rewrite the teal.widgets connected tools (what have to be rewritten). panel/card vs collapsible vs accordion (collapsible for teal.widgets and accordion for teal.reporter) (IN PROGRESS)
[x] check the custom bootstrap related css rules (should not be many).
[ ] change the ggplot color palette at the same time as the app theme - protect against white plot on the dark theme (IN FUTURE)
[ ] teal.gallery and support for bslib::run_with_themer

collapsible - data-toggle ->(in 5) data-bs-toggle

teal.widgets::panel_status - not used (and accordion js)

tabPanel different active structure class for a not li , so broken conditionalPanel for the missing data module

Polkas commented 2 years ago

with have 4 different bootstrap versions, NULL (default) and bslib 3, 4, 5 where NULL a 3 should be the same but have small differences.

options("teal.bs_theme" = NULL)
teal.gallery::launch_app("APPNAME")
options("teal.bs_theme" = bslib::bs_theme(version = "3"))
teal.gallery::launch_app("APPNAME")
options("teal.bs_theme" = bslib::bs_theme(version = "4"))
teal.gallery::launch_app("APPNAME")
options("teal.bs_theme" = bslib::bs_theme(version = "5"))
teal.gallery::launch_app("APPNAME")

Sample apps:

> knitr::kable(data.frame(app = teal.gallery::list_apps(), "NULL" = "", "3" = "", "4" = "", "5" = ""))

app	X3	X4	X5
GLOBAL		lack of panel	lack of panel and changed collapsible (data-bs prefix), filter panel checkbox bars
basic-teal
CDSE-MAE-connector
DataSetDB-MAE-connector
early-dev
efficacy	binary_outcome km tte (switchInput)
entimICE-DDL
exploratory	variable_browser and bivariate (switchInput)	missing_data	missing_data
longitudinal		boxplot and density broken width
patient-profile
python		Variable Browser Error: attempt to select less than one element in get1index	Variable Browser Error: attempt to select less than one element in get1index
RNA-seq
safety		Error in graphics::plot.new: figure margins too large (on init)

Polkas commented 2 years ago

I have a kind request to you (@insightsengineering/nest-core-dev ), to test the current state of the bootstrap application.

when on 709_bs345@main - staged.dependencies::dependency_table() |> staged.dependencies::install_deps(install_direction = "all")
read the https://github.com/insightsengineering/teal/blob/709_bs345%40main/vignettes/teal-bs-themes.Rmd

Run a few sample apps like:


# Optionally extend the  `bslib::bs_theme` options, like custom theme
options("teal.bs_theme" = NULL)
teal.gallery::launch_app("APPNAME")

remotes::install_github("https://github.com/dreamRs/shinyWidgets@main") options("teal.bs_theme" = bslib::bs_theme(version = "3")) teal.gallery::launch_app("APPNAME")

options("teal.bs_theme" = bslib::bs_theme(version = "4")) teal.gallery::launch_app("APPNAME")

options("teal.bs_theme" = bslib::bs_theme(version = "5")) teal.gallery::launch_app("APPNAME")



And finally give your response.

donyunardi commented 2 years ago

When reading the vignette, I got this error message when trying out the example code for run_with_themer:

donyunardi commented 2 years ago

Quick glance every theme and it looks great!

I was reviewing exploratory app from teal.gallery with bs=5 theme, and there are couple things that I noticed:

Module

In FileViewer, I see Couldn't load plugin' message when I selectedpdf` encoding.

Filter Panel

When deselecting all options, the selectizeInput size changes and it's hard to see the available selection to choose.

The blue background is not in the position with the selection.

Dataset name is not in bold red. There is no padding between filter category.

Encoding

The selectInput box is either transparent or has the same color with background The font color for dataset name (i.e. ADSL)is not red. The Dataset field is not on a new line.

I'm not sure if these are all just side-effect of new bs theme or if this teal's css related.

Polkas commented 2 years ago

@dony, I just fix the vignette example. The interactive theming for Bootstrap 3 isn't supported, I added the options() call at the top of this example. BTW if you use e.g. bslib::bs_theme(version = "3", bootswatch = "superhero") it will work, so theming in bs3 is limited only for the regular mode.

Polkas commented 2 years ago

All of the items have to be investigated. I do not now yet how many of them I will solve that is why I still not created issues.

(RStudio problem and already on main) In FileViewer, this is sth new for me. I could not reproduce it. @nikolas-burkoff and @mhallal1 could reproduce it only in the RStudio Viewer which should not a problem for us. @donyunardi please run the app in the regular browser and confirm if this is working smoothly for u.
(FIXED) selectInput overflow in bs5
(LEAVE) I added a temporary fix there. It will never be perfect (using current design) even now on bs3.

4a. (LEAVE) code tag is rendered with the css rules, so it is not a problem for me. 4b. (FIXED) The padding case, is truly a lack of margin between well class elements which are interpreted as card item which do not have margin by default. Each element in the filter panel have a proper id and classes so could be eaisly updated, like options("teal.bs_theme" = bslib::bs_theme(version = "5") |> bslib::bs_add_rules(sass::as_sass("#teal_secondary_col .well {margin: 10px 0;}"))). I added a css rule there but not sure if we should leave it as it was.

(LEAVE) I know about it but it is a problem only for the default theme in the higher bootstrap. In my opinion is not a problem.

donyunardi commented 2 years ago

Just want to add that the FileViewer module, the pdf view part, runs smoothly once I run the app in regular browser. Thanks for the suggestion!

insightsengineering / teal