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

Review teal documentation before release #815

Closed
donyunardi opened this issue Mar 24, 2023 · 14 comments · Fixed by #819
Closed

Review teal documentation before release #815

donyunardi opened this issue Mar 24, 2023 · 14 comments · Fixed by #819
Assignees
Labels

Comments

@donyunardi
Copy link
Contributor

donyunardi commented Mar 24, 2023

Summary

Related with insightsengineering/nestdevs-tasks#13
In preparation to release teal, we need to review/assess all documentation in the package.

Acceptance Criteria

  • Review roxygen examples on all functions
    • Test examples
    • If it's using chunk, update to qenv.
    • Review vignettes on these topics only:
    • Review any content around qenv
    • Review any content around chunks (i.e. make sure the right language around deprecation)
    • Review any content around shinyvalidate
  • Update NEWS
  • Update README (if applicable)
  • Review DESCRIPTION file (i.e. dependency versions, etc)
@asbates
Copy link
Contributor

asbates commented Mar 27, 2023

I'm going to make a running list of notes here.

  • There are reference to chunks in R/get_rcode.R and it's associated test file. The functions here have chunks deprecation warnings.
  • The vignette "Adding support for Reporting to custom modules" uses qenv in an example. I think we should add a short description of qenv, point to teal.code, or remove the qenv usage possible.
  • Example in ?example_module errors when adding a filter. See details and reprex below.
  • For the example in ?get_rcode I would add a comment explaining where this example code would be run.
  • The example app in ?init errors on launch. It's the same error message as with example_module above.
  • Example app in ?ui_nested_tabs fails to launch. "Error in inherits: argument "reporter" is missing, with no default"
  • The examples in module_tabs_with_filters uses :::. These shouldn't be used.
  • I don't understand the purpose of the second example in module_tabs_with_filters. If it doesn't add anything helpful it should be removed. Also it doesn't work right (see below).
  • Similarly for module_teal. It uses ::: and I'm not sure the example is helpful.
  • I don't think the example in modules_debugging works as intended.
  • validate_has_variable example needs a variable that is not in ADSL.
  • I don't think the validate_n_levels example is working correctly.

@asbates
Copy link
Contributor

asbates commented Mar 28, 2023

?example_module example app errors when adding a filter. I think this may be limited to numeric filters, specifically rendering the histogram above the slider.

code:

 devtools::load_all("teal")

 app <- init(
   data = teal_data(
     dataset("IRIS", iris),
     dataset("MTCARS", mtcars)
   ),
   modules = example_module()
 )
   shinyApp(app$ui, app$server)

Add a filter for Sepal.Width in iris.

@nikolas-burkoff
Copy link
Contributor

Checking you are on the filter refactor branch of teal and teal.slice (as I assume that's what will be released)

@donyunardi
Copy link
Contributor Author

donyunardi commented Mar 29, 2023

Checking you are on the filter refactor branch of teal and teal.slice (as I assume that's what will be released)

Actually, we're only focusing on the one on main branch for this release.

@asbates
Copy link
Contributor

asbates commented Mar 29, 2023

Second example in module_tabs_with_filters doesn't work. If I add a filter to the left app, it gets added to the filter panel in the right app, twice.

Steps to reproduce

  1. Launch the app
  2. Add a filter on the left app. For example, SEX in ADSL.

image

@donyunardi
Copy link
Contributor Author

donyunardi commented Mar 30, 2023

I was able to reproduce your experience, however, I was suspicious about this part:

#' datasets1 <- datasets2 <- datasets

So I updated to this:

datasets1 <- teal:::get_dummy_datasets()
datasets2 <- teal:::get_dummy_datasets()

And now I can see it's working:
image

Perhaps this have something to do with object addresses? or maybe the fact that get_dummy_datasets() returns an R6 class (reference)?

@donyunardi
Copy link
Contributor Author

donyunardi commented Mar 30, 2023

?example_module example app errors when adding a filter. I think this may be limited to numeric filters, specifically rendering the histogram above the slider.

code:

 devtools::load_all("teal")

 app <- init(
   data = teal_data(
     dataset("IRIS", iris),
     dataset("MTCARS", mtcars)
   ),
   modules = example_module()
 )
   shinyApp(app$ui, app$server)

Add a filter for Sepal.Width in iris.

I was able to run it the example:

image

Any chance you were not using the main branch version of teal.slice?

@asbates
Copy link
Contributor

asbates commented Mar 30, 2023

Any chance you were not using the main branch version of teal.slice?

Just double checked. I'm on main for both teal and teal slice. And used devtools::load_all() for both packages to ensure the correct branch. Still getting the error.

There's clearly something weird going on though. I've just run some more apps and numeric filters are giving errors.

I also tried re-running staged.dependencies and using library(teal). No dice.

@donyunardi
Copy link
Contributor Author

@chlebowa @nikolas-burkoff @BLAZEWIM any chance you guys can try this?

@nikolas-burkoff
Copy link
Contributor

It seems to work ok for me:
image

I wonder if there's a difference in an external dependency?

@BLAZEWIM
Copy link
Contributor

BLAZEWIM commented Mar 31, 2023

Confirm, works for me
image

@chlebowa
Copy link
Contributor

Only works for me if I load_all teal.slice. When teal.slice is attached with library I get this:

Listening on http://127.0.0.1:3399
Warning: Error in ..stacktraceon..: data all is not available
  3: runApp
  2: print.shiny.appobj
  1: <Anonymous>

teal is installed from refactor branch and attached with library.

@asbates
Copy link
Contributor

asbates commented Mar 31, 2023

Regarding the errors with the range slider plot, I think this is an ggplot2/rlang issue. I had ggplot2 3.4.1 which correctly requires rlang >= 1.1.0. However for some reason I still had rlang 1.0.6 installed.

I'll go back through the examples and update the bullets above.

@donyunardi
Copy link
Contributor Author

Only works for me if I load_all teal.slice. When teal.slice is attached with library I get this:

Listening on http://127.0.0.1:3399
Warning: Error in ..stacktraceon..: data all is not available
  3: runApp
  2: print.shiny.appobj
  1: <Anonymous>

teal is installed from refactor branch and attached with library.

We're focusing on main branch for now.

asbates added a commit that referenced this issue Apr 5, 2023
Fixes a few documentation issues that need to be addressed for release.
Note that not all items from #815 are in this PR. Some of them need
discussion, so I opened #818.

Closes #815
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 a pull request may close this issue.

5 participants