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

443 rcycle@main #637

Merged
merged 40 commits into from
Jun 15, 2022
Merged
Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
3603e96
tests
Polkas May 20, 2022
d38e539
Revert "tests"
May 20, 2022
a8b62f6
callable
May 20, 2022
ff80b88
Merge branch 'main' into 443_rcycle@main
May 20, 2022
11f5979
Update module_nested_tabs.R
May 20, 2022
9641494
rm shinyUI
May 23, 2022
dd18019
old comment
Polkas May 23, 2022
4dc2c39
old comment
Polkas May 23, 2022
622298e
Merge branch 'main' into 443_rcycle@main
May 24, 2022
49172ca
pass tests
Polkas May 24, 2022
48b4705
trigger test
Polkas May 24, 2022
c10460d
roxygen2
Polkas May 24, 2022
6714ce1
Dawid reco
Polkas May 24, 2022
78253d2
Merge branch 'main' into 443_rcycle@main
May 24, 2022
e9a7256
spelling
Polkas May 24, 2022
8bd492a
Merge branch 'main' into 443_rcycle@main
May 25, 2022
a993b82
test
Polkas May 25, 2022
4e7b7a1
test
Polkas May 25, 2022
24287d2
Merge branch 'main' into 443_rcycle@main
May 25, 2022
bffed58
full scope
Polkas May 25, 2022
d2ba775
test
Polkas May 25, 2022
80a5edd
better event js
Polkas May 25, 2022
edd249b
better event js
Polkas May 25, 2022
21cdb5f
better event js
Polkas May 25, 2022
4186205
comment
Polkas May 25, 2022
9302e18
more direct selector
Polkas May 25, 2022
201fbf3
better js code
Polkas May 26, 2022
cb249d9
nested tabs
Polkas May 26, 2022
24fc94f
not export
Polkas May 26, 2022
7dd56d6
DRY
Polkas May 26, 2022
9af939c
Merge branch 'main' into 443_rcycle@main
mhallal1 May 27, 2022
0e339a5
Update R/module_teal.R
May 27, 2022
f49e165
Update R/module_nested_tabs.R
May 27, 2022
efedccd
Update R/module_nested_tabs.R
May 27, 2022
5798846
full input cycle
Polkas May 30, 2022
9fad112
Dawid reco
Polkas May 30, 2022
7a78a84
Merge branch 'main' into 443_rcycle@main
Jun 10, 2022
5588fc0
rm the note in the vignette
Jun 13, 2022
d06502e
Merge branch 'main' into 443_rcycle@main
Jun 15, 2022
da4f8ed
Update NEWS.md
Jun 15, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 34 additions & 16 deletions R/module_nested_tabs.R
Original file line number Diff line number Diff line change
Expand Up @@ -53,18 +53,19 @@ ui_nested_tabs.default <- function(id, modules, datasets, depth = 0L) {
stop("Modules class not supported: ", paste(class(modules), collapse = " "))
}

#' @rdname ui_nested_tabs
#' @export

#' @keywords internal
ui_nested_tabs.teal_modules <- function(id, modules, datasets, depth = 0L) {
ui_nested_tabs_base <- function(id, modules, datasets, depth = 0L, selected) {
ns <- NS(id)
do.call(
tabsetPanel,
c(
# by giving an id, we can reactively respond to tab changes
list(
id = ns("active_tab"),
type = if (modules$label == "root") "pills" else "tabs"
type = if (modules$label == "root") "pills" else "tabs",
# some random name to select nothing
selected = selected
),
lapply(
names(modules$children),
Expand All @@ -80,6 +81,18 @@ ui_nested_tabs.teal_modules <- function(id, modules, datasets, depth = 0L) {
)
}

#' @keywords internal
ui_nested_tabs_init <- function(id, modules, datasets, depth = 0L) {
ui_nested_tabs_base(id, modules, datasets, depth, "none")
}

#' @rdname ui_nested_tabs
#' @export
#' @keywords internal
ui_nested_tabs.teal_modules <- function(id, modules, datasets, depth = 0L) {
ui_nested_tabs_base(id, modules, datasets, depth, NULL)
}

#' @rdname ui_nested_tabs
#' @export
#' @keywords internal
Expand Down Expand Up @@ -151,6 +164,7 @@ srv_nested_tabs.teal_modules <- function(id, datasets, modules, reporter) {

get_active_module <- reactive({
if (length(modules$children) == 1L) {
req(input$active_tab)
# single tab is active by default
modules_reactive[[1]]()
} else {
Expand All @@ -176,18 +190,22 @@ srv_nested_tabs.teal_module <- function(id, datasets, modules, reporter) {
)
)

modules$server_args <- teal.transform::resolve_delayed(modules$server_args, datasets)
is_module_server <- isTRUE("id" %in% names(formals(modules$server)))
module_reactive <- reactive({
modules$server_args <- teal.transform::resolve_delayed(modules$server_args, datasets)
is_module_server <- isTRUE("id" %in% names(formals(modules$server)))
args <- c(list(id = id, datasets = datasets), modules$server_args)

args <- c(list(id = id, datasets = datasets), modules$server_args)
if (is_reporter_used(modules)) {
args <- c(args, list(reporter = reporter))
}
if (is_reporter_used(modules)) {
args <- c(args, list(reporter = reporter))
}

if (is_module_server) {
do.call(modules$server, args)
} else {
do.call(callModule, c(args, list(module = modules$server)))
}
modules
})

if (is_module_server) {
do.call(modules$server, args)
} else {
do.call(callModule, c(args, list(module = modules$server)))
}
reactive(modules)
module_reactive
}
2 changes: 1 addition & 1 deletion R/module_tabs_with_filters.R
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ ui_tabs_with_filters <- function(id, modules, datasets) {
filter_and_info_ui <- datasets$ui_filter_panel(ns("filter_panel"))

# modules must be teal_modules, not teal_module; otherwise we will get the UI and not a tabsetPanel of UIs
teal_ui <- ui_nested_tabs(ns("root"), modules = modules, datasets)
teal_ui <- ui_nested_tabs_init(ns("root"), modules = modules, datasets)

filter_panel_btn <- tags$li(
style = "flex-grow : 1;",
Expand Down
37 changes: 19 additions & 18 deletions R/module_teal.R
Original file line number Diff line number Diff line change
Expand Up @@ -89,17 +89,15 @@ ui_teal <- function(id,
)
)

res <- shinyUI(
fluidPage(
title = title,
include_teal_css_js(),
tags$header(header),
tags$hr(style = "margin: 7px 0;"),
shiny_busy_message_panel,
splash_ui,
tags$hr(),
tags$footer(div(footer, textOutput(ns("identifier"))))
)
res <- fluidPage(
title = title,
include_teal_css_js(),
tags$header(header),
tags$hr(style = "margin: 7px 0;"),
shiny_busy_message_panel,
splash_ui,
tags$hr(),
tags$footer(div(footer, textOutput(ns("identifier"))))
)
return(res)
}
Expand Down Expand Up @@ -216,13 +214,17 @@ srv_teal <- function(id, modules, raw_data, filter = list()) {
where = "beforeEnd",
# we put it into a div, so it can easily be removed as a whole, also when it is a tagList (and not
# just the first item of the tagList)
ui = div(ui_tabs_with_filters(
session$ns("main_ui"),
modules = modules,
datasets = datasets_reactive()
)),
ui = div(
# This id is linked with a inst/js/init.js code which activates the app
id = "teal_main_modules_ui",
ui_tabs_with_filters(
session$ns("main_ui"),
modules = modules,
datasets = datasets_reactive()
)
),
# needed so that the UI inputs are available and can be immediately updated, otherwise, updating may not
# have any effect as they are ignored when not present, see note in `module_add_filter_variable.R`
# have any effect as they are ignored when not present
immediate = TRUE
)

Expand All @@ -232,7 +234,6 @@ srv_teal <- function(id, modules, raw_data, filter = list()) {

# must make sure that this is only executed once as modules assume their observers are only
# registered once (calling server functions twice would trigger observers twice each time)
# `once = TRUE` ensures this
active_module <- srv_tabs_with_filters(
id = "main_ui",
datasets = datasets_reactive(),
Expand Down
27 changes: 27 additions & 0 deletions inst/js/init.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,30 @@

// this code alows the show R code "copy to clipbaord" button to work
var clipboard = new ClipboardJS('.btn[data-clipboard-target]');

function wait_for_element(selector) {
return new Promise(resolve => {
let init_check = document.querySelector(selector);
if (init_check) {
return resolve(init_check);
}

const observer = new MutationObserver(() => {
let obs_check = document.querySelector(selector);
if (obs_check) {
resolve(obs_check);
observer.disconnect();
}
});

observer.observe(document.body, {
childList: true,
subtree: true
});
});
}

wait_for_element('div#teal_main_modules_ui').then(() => {
$("div#teal_main_modules_ui a[data-toggle='tab']")[0].click();
});
Comment on lines +9 to +33
Copy link
Contributor

@kpagacz kpagacz May 27, 2022

Choose a reason for hiding this comment

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

Suggested change
function wait_for_element(selector) {
return new Promise(resolve => {
let init_check = document.querySelector(selector);
if (init_check) {
return resolve(init_check);
}
const observer = new MutationObserver(() => {
let obs_check = document.querySelector(selector);
if (obs_check) {
resolve(obs_check);
observer.disconnect();
}
});
observer.observe(document.body, {
childList: true,
subtree: true
});
});
}
wait_for_element('div#teal_main_modules_ui').then(() => {
$("div#teal_main_modules_ui a[data-toggle='tab']")[0].click();
});
const observer = new MutationObserver(() => {
if (document.querySelector("div#teal_main_modules_ui")) {
$("div#teal_main_modules_ui a[data-toggle='tab']")[0].click();
observer.disconnect();
}
});
observer.observe(document.body, {
childList: true,
subtree: true,
});

Here is my proposed refactor that keeps the spirit of the original and does away with some of the bloat. I have troubles testing this, but once launched it does click on the first tab. I do not know how to test the business logic properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice update, still I think the current code is more clear as sb know that we are wait_for_element to click sth.
The wait_for_element function could be useful in the future, and it is only 20 lines of code.

If you want to test business logic you should run any of our apps and the shiny input should contains all variables from the first line of any of the executed module server.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is what I did while testing and there was no change between the current state of the pr and my proposed solution when comparing available values of input from a browser put in the first line of the scatterplot module.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you are worried about the readability, then we can change the name of the observer for something meaningful like: tealTabsObserver or if needed a very verbose: clickFirstTealTabWhenAvailable XD


32 changes: 17 additions & 15 deletions tests/testthat/test-module_nested_tabs.R
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
filtered_data <- teal.slice:::FilteredData$new()
filtered_data$set_dataset(teal.data::dataset(dataname = "iris", x = head(iris)))

test_module1 <- module(
label = "test1",
ui = function(id, ...) NULL,
Expand Down Expand Up @@ -33,23 +34,24 @@ testthat::test_that("srv_nested_tabs throws error if reporter is not inherited f
})

# server -------
testthat::test_that("passed shiny module is initialized", {
testthat::expect_message(
shiny::testServer(
app = srv_nested_tabs,
args = list(
id = "test",
datasets = filtered_data,
modules = modules(test_module1),
reporter = teal.reporter::Reporter$new()
),
expr = NULL
),
"1"
testthat::test_that("passed shiny module is initialized, empty as is delayed", {
testthat::expect_true(
is.null(
shiny::testServer(
app = srv_nested_tabs,
args = list(
id = "test",
datasets = filtered_data,
modules = modules(test_module1),
reporter = teal.reporter::Reporter$new()
),
expr = NULL
)
)
)
})

testthat::test_that("nested teal-modules are initialized", {
testthat::test_that("nested teal-modules are initialized, empty as is delayed", {
out <- testthat::capture_messages(
shiny::testServer(
app = srv_nested_tabs,
Expand All @@ -65,7 +67,7 @@ testthat::test_that("nested teal-modules are initialized", {
expr = NULL
)
)
testthat::expect_identical(out, c("1\n", "2\n", "3\n", "4\n"))
testthat::expect_identical(out, character(0))
})


Expand Down
55 changes: 54 additions & 1 deletion tests/testthat/test-module_tabs_with_filters.R
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,22 @@ filtered_data$set_dataset(teal.data::dataset(dataname = "mtcars", x = head(mtcar

test_module1 <- module(
label = "iris tab",
ui = function(id, ...) NULL,
server = function(id, datasets) moduleServer(id, function(input, output, session) message("1")),
filters = "iris"
)

test_module2 <- module(
label = "mtcars tab",
ui = function(id, ...) NULL,
server = function(id, datasets) moduleServer(id, function(input, output, session) message("2")),
filters = "mtcars"
)

test_module3 <- module(
label = "mtcars tab2",
ui = function(id, ...) NULL,
server = function(id, datasets) moduleServer(id, function(input, output, session) message("3")),
filters = "mtcars"
)

Expand All @@ -18,6 +30,46 @@ testthat::test_that("srv_tabs_with_filters throws error if reporter is not of cl
)
})

testthat::test_that("passed shiny module is initialized when its tab is activated (clicked)", {
testthat::expect_message(
shiny::testServer(
app = srv_tabs_with_filters,
args = list(
id = "test",
datasets = filtered_data,
modules = modules(test_module3),
filter = list(),
reporter = teal.reporter::Reporter$new()
),
expr = {
session$setInputs(`root-active_tab` = "mtcars_tab")
}
),
"3"
)
})

testthat::test_that("passed shiny modules are initialized when their tab is activated (clicked)", {
out <- testthat::capture_messages(
shiny::testServer(
app = srv_tabs_with_filters,
args = list(
id = "test",
datasets = filtered_data,
modules = modules(test_module1, test_module2, test_module3),
filter = list(),
reporter = teal.reporter::Reporter$new()
),
expr = {
session$setInputs(`root-active_tab` = "iris_tab")
session$setInputs(`root-active_tab` = "mtcars_tab")
session$setInputs(`root-active_tab` = "mtcars_tab2")
}
)
)
testthat::expect_identical(out, c("1\n", "2\n", "3\n"))
})

testthat::test_that("active_datanames() returns dataname from single tab", {
shiny::testServer(
app = srv_tabs_with_filters,
Expand All @@ -29,6 +81,7 @@ testthat::test_that("active_datanames() returns dataname from single tab", {
reporter = teal.reporter::Reporter$new()
),
expr = {
session$setInputs(`root-active_tab` = "iris_tab")
testthat::expect_identical(active_datanames(), "iris")
}
)
Expand All @@ -45,7 +98,7 @@ testthat::test_that("active_datanames() returns dataname from active tab after c
reporter = teal.reporter::Reporter$new()
),
expr = {
testthat::expect_error(active_datanames()) # to trigger active_module
testthat::expect_error(active_datanames())
session$setInputs(`root-active_tab` = "iris_tab")
testthat::expect_identical(active_datanames(), "iris")
session$setInputs(`root-active_tab` = "mtcars_tab")
Expand Down