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

use roxy.shinylive #775

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

use roxy.shinylive #775

wants to merge 29 commits into from

Conversation

pawelru
Copy link
Contributor

@pawelru pawelru commented Aug 20, 2024

test with insightsengineering/roxy.shinylive#1

Implementation of roxy.shinylive.

Please run pkgdown::build_site() to see the difference.
image

Extra:

  • for a few examples: remove dependency on teal.widgets so that the example code is simpler
  • for a few examples: replace @examples with @examplesIf. This is for our incorrect implementation of soft dependencies
  • add a new "Playground" section in the README with links to the shinylive
  • include shinylive iframe in the vignettes

Copy link
Contributor

github-actions bot commented Aug 20, 2024

badge

Code Coverage Summary

Filename                      Stmts    Miss  Cover    Missing
--------------------------  -------  ------  -------  ------------------------------------
R/tm_a_pca.R                    827     827  0.00%    109-1069
R/tm_a_regression.R             773     773  0.00%    152-1030
R/tm_data_table.R               185     185  0.00%    102-341
R/tm_file_viewer.R              173     173  0.00%    48-256
R/tm_front_page.R               133     122  8.27%    74-232
R/tm_g_association.R            330     330  0.00%    136-538
R/tm_g_bivariate.R              672     410  38.99%   303-769, 810, 921, 938, 956, 967-989
R/tm_g_distribution.R          1048    1048  0.00%    127-1313
R/tm_g_response.R               351     351  0.00%    153-577
R/tm_g_scatterplot.R            722     722  0.00%    235-1058
R/tm_g_scatterplotmatrix.R      278     259  6.83%    174-481, 542, 556
R/tm_missing_data.R            1069    1069  0.00%    93-1318
R/tm_outliers.R                 985     985  0.00%    131-1260
R/tm_t_crosstable.R             251     251  0.00%    140-439
R/tm_variable_browser.R         830     825  0.60%    91-1083, 1121-1305
R/utils.R                        99      96  3.03%    82-267
R/zzz.R                           2       2  0.00%    2-3
TOTAL                          8728    8428  3.44%

Diff against main

Filename      Stmts    Miss  Cover
----------  -------  ------  --------
TOTAL             0       0  +100.00%

Results for commit: 006c1f2

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@pawelru pawelru added the core label Aug 20, 2024
Copy link
Contributor

github-actions bot commented Aug 20, 2024

Unit Tests Summary

  1 files   22 suites   13m 29s ⏱️
147 tests 147 ✅ 0 💤 0 ❌
479 runs  479 ✅ 0 💤 0 ❌

Results for commit 006c1f2.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Aug 20, 2024

Unit Test Performance Difference

Test Suite $Status$ Time on main $±Time$ $±Tests$ $±Skipped$ $±Failures$ $±Errors$
shinytest2-tm_a_pca 💔 $115.37$ $+4.94$ $0$ $0$ $0$ $0$
shinytest2-tm_a_regression 💔 $52.53$ $+2.51$ $0$ $0$ $0$ $0$
shinytest2-tm_file_viewer 💔 $36.17$ $+1.14$ $0$ $0$ $0$ $0$
shinytest2-tm_t_crosstable 💔 $31.57$ $+2.29$ $0$ $0$ $0$ $0$
Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
shinytest2-tm_t_crosstable 💔 $10.51$ $+2.39$ e2e_tm_t_crosstable_Change_plot_settings

Results for commit 070ee04

♻️ This comment has been updated with latest results.

#' interactive <- function() TRUE
#' {{ next_example }}
# nolint start: line_length_linter.
#' @examplesIf require("ggpmisc", quietly = TRUE) && require("ggpp", quietly = TRUE) && require("goftest", quietly = TRUE) && require("MASS", quietly = TRUE) && require("broom", quietly = TRUE)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI: this line of code would be included in the application code. It's important to use only library() or require() because those two are currently shimmed by {webr}(*). In particular: we cannot use requireNamespace() (**). I created a new task for {webr} to add this. Until this happen - let's stick to the require().

(*) shimming is overwriting in this context and {webr} will do install (if needed) & load every time you run these
(**) Actually we can but then we would have to include additional library() or require() in the shinyliveexample section which seems to be redundant.

Signed-off-by: Pawel Rucki <12943682+pawelru@users.noreply.github.com>
@donyunardi
Copy link
Contributor

The idea of adding a shinylive session for examples is pretty cool, thanks for showcasing this!

I like the idea that users can see the examples in a Shiny context right away when they read our online documentation and I think this is a valid use case for a shiny oriented R package and can accelerate learning process.

Here are couple thoughts as I play around with this:

The edited .Rd file

The portion being added to the .Rd file looks very straightforward. However, I'm wondering what would happen if the examples are complex or if we have a lot of Shiny app examples for one function. Could this result in a bloated .Rd file that is no longer lightweight? If so, is there any risk that we should consider?

I also tried to see how the documentation would look when opened locally, i.e. ?tm_g_scatterplot. From the screenshot below, you can see that RStudio IDE tries to render the iframe. Is this the behavior we want? Personally, it would be better to just give link to the shinylive.io URL if user wishes to see the examples in action. The default help size window in RStudio is too small for shinylive session and I had to resize it for better shinylive experience.

image

PS: Not sure if this happened to you, but once when I ran ?tm_g_scatterplot, it opened around 15+ tabs in my Chrome browser, all heading to the shinylive URL. I tried to recreate this but couldn’t.

shinylive teal app examples

After running pkgdown::build_site(), I noticed that some of the buttons in the teal app inside the iframe aren't working, like the hamburger menu, snapshot manager, filter collapse arrow, and scrolling down the teal app.

See my attempt to click perform these actions:
https://github.com/user-attachments/assets/03a7485c-707a-41f3-a0c4-1273aeee6588

Since we're using an iframe solution, I'm not sure how easy it would be to troubleshoot this. I started to google around and so far I have the impression that it might be related to the CSS/JavaScript cross between the webpage in the iframe and the main page. If we can't find a good solution to this, my only concern is that users might not have the best experience exploring teal examples within the iframe boundaries. Maybe that's okay since you're also allowing users to try the example directly on the shinylive.io website.

Overall, I think this is promising!

@pawelru
Copy link
Contributor Author

pawelru commented Sep 4, 2024

Thanks for your comments - please find answers below

I'm wondering what would happen if the examples are complex or if we have a lot of Shiny app examples for one function.

I somewhat anticipated this in the whole design and I made it as an opt-in - one needs to include something in order for this section to appear in the docs. So if an example is really complex then one can choose not to have it in shinylive by not including @examplesShinylive at all.
Please also note when writting the docs we can have multiple @examples section that will be eventually merged together. This is a base roxygen feature. Something like that:

#' foo title
#' @param ...
#' @examples
#' <examples part 1>
#' @examples
#' <examples part 2>
#' @examples
#' <examples part 3>
#' ...
foo <- ...

This allows us to include part of examples:

#' foo title
#' @param ...
#' @examples
#' <examples part 1> (not included)
#' @examplesShinylive
#' @examples
#' <examples part 2> (included)
#' @examples
#' <examples part 3> (not included)
#' ...
foo <- ...

I think it's a valid use case and I included it in the documentation here

Could this result in a bloated .Rd file that is no longer lightweight? If so, is there any risk that we should consider?

In terms of size it's pretty lightweight. Looking into a random file in tmg it grows by exactly 9 lines.
I was thinking about edge cases and potential risks. One thing could be rendering into the pdf format and I have included this so that iframe is included only for html type of output - if{html}... here.
Another thing could be rendering the docs in other services like rdocumentation or r-universe. This should be good but I was unable to really test it.
One more important thing is to CRAN release. tmg is on cran and for this to be re-released we must release roxy.shinylive before. This would be the first release on CRAN so it might take a while. That's why I think it would be good to start it relatively soon not to block upstream releases.
I have not identified any other risks to be honest.

From the screenshot below, you can see that RStudio IDE tries to render the iframe. Is this the behavior we want? Personally, it would be better to just give link to the shinylive.io URL if user wishes to see the examples in action.

Funnily - I have started with just a link and then extended this by iframe. I think it would be nice to include this somehow but the challenge is that the available space might not be enough.

The default help size window in RStudio is too small for shinylive session and I had to resize it for better shinylive experience.

Point taken. I can see that 125% width might not be a good call. Let me experiment with this more.

After running pkgdown::build_site(), I noticed that some of the buttons in the teal app inside the iframe aren't working, like the hamburger menu, snapshot manager, filter collapse arrow, and scrolling down the teal app.

Hmmm that might be an evidence of a bad JS on our side. I have a few similar findings and I expect more to come. But that's actually a good thing as this would allow us to detect more bugs on our side.

my only concern is that users might not have the best experience exploring teal examples within the iframe boundaries. Maybe that's okay since you're also allowing users to try the example directly on the shinylive.io website.

Point taken. Let me have a few tries to improve this. If it cannot be done correctly I'm fine to come back to the solution without iframe. No pressure on this

@pawelru
Copy link
Contributor Author

pawelru commented Sep 5, 2024

Hey @donyunardi please have a look now. I have pushed a few changes including visibility, width and fixing all the NOTES that I haven't noticed earlier.

Copy link
Contributor

@donyunardi donyunardi left a comment

Choose a reason for hiding this comment

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

Thanks for the update.

I found couple more things:

z-index

I discovered why I couldn't interact with the filter panel: the iframe's z-index was lower than the navigation links.

After increasing the z-index, I was able to interact with the filter panel, as shown here:
image

However, increasing the z-index causes the navigation links to be placed behind the iframe, which might prevent users from seeing them when playing with the app. Personally, I think that's okay, but let me know what you think.

No Data in the teal app

Some teal app does not have any data:
image

If I click to the shinylive.io link on the page, I see this error message:
image

The error is:

Cannot read properties of null (reading 'deps')

Do you get the same problem?

Signed-off-by: Pawel Rucki <12943682+pawelru@users.noreply.github.com>
@pawelru
Copy link
Contributor Author

pawelru commented Sep 17, 2024

z-index

Great suggestion! Thank you! Implemented in insightsengineering/roxy.shinylive@1181b1e and here in c5cc45f

Cannot read properties of null

Known issue. I reported this in posit-dev/r-shinylive#128. It's already fixed but not yet fully deployed. I would suggest to go with this as is and wait for a deploy of the fixed version

Copy link
Contributor

@donyunardi donyunardi left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, I think this looks great!

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.

2 participants