Add teal app modifiers and deprecate init args

[vedhav]

Closes #1310 and #1143

Changes

• The title, header, and footer are soft deprecated in favor of using add_title, add_header, add_footer, and add_landing_popup
• Adds a new add_custom_server modifier to add custom server logic to the main shiny app.
• Reverts the usage of landing_popup_module to the previous way by passing it inside the modules
• Bumps R version to 4.1 to start using native pipe |>

Example app with the new app modifiers.

devtools::load_all("teal")

app <- init(
  data = teal_data(IRIS = iris, MTCARS = mtcars),
  modules = modules(
    example_module("Module 1"),
    example_module("Module 2")
  ),
  filter = teal_slices(
    teal_slice(dataname = "IRIS", varname = "Species", selected = "setosa")
  )
) |>
  modify_title("Custom title") |>
  modify_header(
    tags$div(
      h3("Header with a button that can be observed!"),
      actionButton("notify", "Show notification")
    )
  ) |>
  modify_footer("Custom footer") |>
   add_landing_popup(
      content = "Popup content",
      buttons = modalButton("Proceed")
   ) |>
  add_custom_server(function(input, output, session) {
    observeEvent(input$notify, {
      showNotification("Yes, the button works!")
    })
  })

shinyApp(app$ui, app$server)

[github-actions]

Unit Tests Summary

  1 files   27 suites   9m 49s ⏱️
275 tests 256 ✅ 4 💤 4 ❌ 11 🔥
521 runs  499 ✅ 4 💤 7 ❌ 11 🔥

For more details on these failures and errors, see this check.

Results for commit ff480f9.

♻️ This comment has been updated with latest results.

[github-actions]

Unit Test Performance Difference

Test Suite	$Status$	Time on main	$±Time$	$±Tests$	$±Skipped$	$±Failures$	$±Errors$
shinytest2-filter_panel	💚	$41.75$	$-1.28$	$0$	$0$	$+1$	$0$
shinytest2-init	💚	$26.80$	$-1.39$	$-10$	$0$	$+1$	$+1$
shinytest2-landing_popup	💔	$43.82$	$+2.12$	$0$	$0$	$0$	$0$
shinytest2-module_bookmark_manager	💔	$34.53$	$+1.60$	$0$	$0$	$0$	$0$
shinytest2-reporter	💚	$66.54$	$-3.85$	$-1$	$0$	$+3$	$+3$
shinytest2-teal_data_module	💚	$47.03$	$-28.15$	$-3$	$0$	$0$	$+6$
shinytest2-teal_slices	💚	$60.88$	$-6.34$	$-2$	$0$	$+1$	$+1$
shinytest2-wunder_bar	💔	$21.28$	$+1.46$	$0$	$0$	$0$	$0$

Additional test case details

Test Suite	$Status$	Time on main	$±Time$	Test Case
shinytest2-init	💚	$10.33$	$-1.84$	e2e_teal_app_initializes_with_sessionInfo_modal
shinytest2-reporter	💚	$18.43$	$-4.53$	e2e_adding_a_report_card_in_a_module_adds_it_in_the_report_previewer_tab
shinytest2-teal_data_module	💚	$8.20$	$-5.07$	e2e_teal_data_module_gets_removed_after_successful_data_load_when_once_TRUE
shinytest2-teal_data_module	💚	$11.67$	$-8.48$	e2e_teal_data_module_inputs_change_teal_data_object_that_is_passed_to_teal_main_UI
shinytest2-teal_data_module	💚	$7.52$	$-4.41$	e2e_teal_data_module_is_still_visible_after_successful_data_load_when_once_FALSE
shinytest2-teal_data_module	💚	$5.20$	$-2.08$	e2e_teal_data_module_shows_validation_errors
shinytest2-teal_data_module	💚	$8.86$	$-5.66$	e2e_teal_data_module_will_have_a_delayed_load_of_datasets
shinytest2-teal_data_module	💚	$5.59$	$-2.44$	e2e_teal_data_module_will_make_other_tabs_inactive_before_successful_data_load
shinytest2-teal_slices	💚	$37.26$	$-6.01$	e2e_teal_slices_filters_are_initialized_when_module_specific_filters_are_created
shinytest2-wunder_bar	💔	$10.65$	$+1.10$	wunder_bar_srv_clicking_filter_icon_opens_filter_manager_modal

Results for commit f60f4e4

♻️ This comment has been updated with latest results.

[gogonzo]

Some room for improvement

[gogonzo]

should be possible to add a moduleServer

Suggested change

	custom_server(input, output, session)
	if (all(c("input", "output", "session") %in% names(formals(custom_server)))) {
	callModule(custom_server, "wrapper")
	} else if ("id" %in% names(formals(custom_server))) {
	custom_server("wrapper")
	}

[vedhav]

Once we deprecate the id parameter, this level will be a non-shiny-module root and I think it will keep things simpler if we keep it that way.
Do you see value in allowing app developers to pass shiny module ui in header/footer and adding the server module here?

[gogonzo]

I'm not pushing for this. I just realised that we shouldn't call custom_server(input, output, session) but use `callModule(custom_server, "wrapper") instead to separate possible namespace collision.

[vedhav]

There won't be any namespace collision as ui_teal/srv_teal is shiny module with a "teal" namespace and the root level of shiny session namespace is free for the taking.

[gogonzo]

What if inside of the custom server one calls a module with the namespace teal

[gogonzo]

@vedhav ⬆️

[gogonzo]

What if inside of the custom server one calls a module with the namespace teal

@vedhav Sorry I didn't use question mark in the above comment. I hope we continue to discuss this.

There won't be any namespace collision as ui_teal/srv_teal is shiny module with a "teal" namespace and the root level of shiny session namespace is free for the taking.

Yes, there will be namespace collision if somebody calls twice add_custom_server where some ids are duplicated. There will be a collision because if we don't callModule both modules would operate under teal namespace.

My suggestion though set another trap - if one uses add_custom_server twice, then both will be called with an id ="wrapper" and we'll have a collision.

P.S. you've committed my two suggestions where assert_function(custom_server, args = c("input", "output", "session") makes if ("id" %in% names(formals(...))) impossible.

[vedhav]

I applied the suggestion and added an example to call the module server. Please have a look tomorrow!

[gogonzo]

To introduce the pipe operator in the internal code we need to agree to certain style rules within a team. Pipe might be overused to replace a single call which in my opinion is bad as it doesn't bring clarity plus it is a extra sugar to evaluate.

Suggested change

	app <- app |>
	add_landing_popup(
	app <- add_landing_popup(
	app,

[gogonzo]

ui_teal shouldn't have header, footer, title arguments. Deprecation in init only is a half-measure. Please use add_header and add_title in teal::init body instead of passing arguments to ui_teal.

Edit: Same applies to module_teal_with_splash

[m7pr]

If build_app_title going to be removed, maybe use the substitution for build_app_title in here? Or it's going to stay, but will just be unexported?

[vedhav]

I'm on the fence here. I don't mind removing it completely or stop exporting it. I lean towards completely removing it.

[gogonzo]

This is an exported function. We should soft_deprecated it first. It should be also taken away from default ui_teal(title) and ui_teal_with_splash(title).

[m7pr]

Hey, really great start! A food for thought below

• I think this solution would benefit if we can craft a vignette explaining how you can customise teal app
• Just an idea, for another issue: what if we allow user to modify the layout (allow filter panel to be on the left, allow to pass any custom JS) or aesthetics (colours, fonts) of the app if modify_layout/modify_ui function?