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

Add teal app modifiers and deprecate init args #1440

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

vedhav
Copy link
Contributor

@vedhav vedhav commented Jan 7, 2025

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)

@vedhav vedhav added the core label Jan 7, 2025
Copy link
Contributor

github-actions bot commented Jan 7, 2025

Unit Tests Summary

  1 files   27 suites   10m 42s ⏱️
275 tests 271 ✅ 4 💤 0 ❌
537 runs  533 ✅ 4 💤 0 ❌

Results for commit 63065f0.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Jan 7, 2025

Unit Test Performance Difference

Test Suite $Status$ Time on main $±Time$ $±Tests$ $±Skipped$ $±Failures$ $±Errors$
shinytest2-landing_popup 💔 $46.22$ $+1.62$ $0$ $0$ $0$ $0$
shinytest2-teal_slices 💔 $61.15$ $+1.33$ $0$ $0$ $0$ $0$
Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
module_teal 💔 $17.12$ $+1.13$ creation_process_is_invoked_for_teal.lockfile.mode_enabled_and_snapshot_is_copied_to_teal_app.lock_and_removed_after_session_ended
shinytest2-teal_slices 💔 $37.40$ $+1.06$ e2e_teal_slices_filters_are_initialized_when_module_specific_filters_are_created

Results for commit d4f5a06

♻️ This comment has been updated with latest results.

@vedhav vedhav marked this pull request as draft January 7, 2025 08:10
@vedhav vedhav force-pushed the 1310-simplify-init-args branch from 1fb5028 to aa60c14 Compare January 10, 2025 07:59
Copy link
Contributor

@gogonzo gogonzo left a comment

Choose a reason for hiding this comment

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

Some room for improvement

R/init.R Outdated Show resolved Hide resolved
R/init.R Outdated Show resolved Hide resolved
R/init.R Outdated Show resolved Hide resolved
R/init.R Outdated Show resolved Hide resolved
R/init.R Outdated Show resolved Hide resolved
R/init.R Outdated Show resolved Hide resolved
R/init.R Show resolved Hide resolved
R/init.R Outdated Show resolved Hide resolved
@vedhav vedhav force-pushed the 1310-simplify-init-args branch from ef65f87 to 0af8a67 Compare January 10, 2025 09:39
R/init.R Outdated

app$server <- function(input, output, session) {
old_server(input, output, session)
custom_server(input, output, session)
Copy link
Contributor

@gogonzo gogonzo Jan 10, 2025

Choose a reason for hiding this comment

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

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")
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@gogonzo gogonzo Jan 10, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@vedhav vedhav Jan 10, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@vedhav ⬆️

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

R/init.R Outdated Show resolved Hide resolved
vedhav and others added 2 commits January 10, 2025 20:27
R/init.R Outdated Show resolved Hide resolved
R/init.R Show resolved Hide resolved
Comment on lines 85 to 86
app <- app |>
add_landing_popup(
Copy link
Contributor

Choose a reason for hiding this comment

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

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,

R/TealAppDriver.R Show resolved Hide resolved
@gogonzo gogonzo self-assigned this Jan 13, 2025
Copy link
Contributor

@gogonzo gogonzo left a comment

Choose a reason for hiding this comment

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

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

R/init.R Show resolved Hide resolved
R/init.R Outdated
details = "Use `modify_title()` on the teal app object instead."
)
} else {
title <- build_app_title()
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

R/init.R Outdated Show resolved Hide resolved
R/init.R Outdated Show resolved Hide resolved
R/init.R Show resolved Hide resolved
R/init.R Outdated Show resolved Hide resolved
R/init.R Outdated Show resolved Hide resolved
R/init.R Outdated Show resolved Hide resolved
R/init.R Outdated Show resolved Hide resolved
_pkgdown.yml Outdated Show resolved Hide resolved
vedhav and others added 8 commits January 13, 2025 15:35
Signed-off-by: Vedha Viyash <[email protected]>
Co-authored-by: Marcin <[email protected]>
Signed-off-by: Vedha Viyash <[email protected]>
Co-authored-by: Marcin <[email protected]>
Signed-off-by: Vedha Viyash <[email protected]>
Co-authored-by: Marcin <[email protected]>
Signed-off-by: Vedha Viyash <[email protected]>
Co-authored-by: Marcin <[email protected]>
Signed-off-by: Vedha Viyash <[email protected]>
Co-authored-by: Marcin <[email protected]>
Signed-off-by: Vedha Viyash <[email protected]>
Co-authored-by: Marcin <[email protected]>
Signed-off-by: Vedha Viyash <[email protected]>
Co-authored-by: Marcin <[email protected]>
Signed-off-by: Vedha Viyash <[email protected]>
@m7pr
Copy link
Contributor

m7pr commented Jan 13, 2025

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?

@vedhav vedhav marked this pull request as ready for review January 13, 2025 12:02
R/init.R Outdated Show resolved Hide resolved
R/init.R Show resolved Hide resolved
R/init.R Outdated Show resolved Hide resolved
R/init.R Outdated Show resolved Hide resolved
R/init.R Outdated Show resolved Hide resolved
R/init.R Outdated Show resolved Hide resolved
R/init.R Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move landing popup feature back to the modules argument
3 participants