From bb02e4481c7227c129bb378b966f6d956d6b48b1 Mon Sep 17 00:00:00 2001 From: Cam Race <52536248+cjrace@users.noreply.github.com> Date: Thu, 12 Sep 2024 11:57:02 +0100 Subject: [PATCH] Fix external link not hidden (#48) * create a CSS dependency and attach (WIP - not yet attaching correctly) * move the stylesheet to be just on the span * Increment version number to 0.4.1 * add zzz.R file for attaching www resources on package load * whitespace still an issue, added examples * Fixed whitespace issue and updated tests, not updated test_dashboard yet * add dashboard checks for the external link function * Adding names back in? * Running this blind as shinytest2 isn't playing ball locally, but making the cookies name match so snapshots match up --- .github/CONTRIBUTING.md | 36 +++++--- .gitignore | 1 - DESCRIPTION | 2 +- NAMESPACE | 1 + NEWS.md | 5 ++ R/external_link.R | 65 ++++++++++++-- R/zzz.R | 12 +++ README.md | 13 ++- inst/www/css/visually-hidden.css | 10 +++ man/external_link.Rd | 43 +++++++++- .../tests/testthat/test-UI-01-basic_load.R | 7 +- .../tests/testthat/test-UI-02-cookies.R | 7 +- .../tests/testthat/test-UI-03-external_link.R | 86 +++++++++++++++++++ tests/test_dashboard/ui.R | 42 +++++++++ .../www/dfe_shiny_gov_style.css | 16 ++-- tests/testthat/test-external_link.R | 41 +++++++-- 16 files changed, 336 insertions(+), 51 deletions(-) create mode 100644 R/zzz.R create mode 100644 inst/www/css/visually-hidden.css create mode 100644 tests/test_dashboard/tests/testthat/test-UI-03-external_link.R diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md index b5cec5d..25597c9 100644 --- a/.github/CONTRIBUTING.md +++ b/.github/CONTRIBUTING.md @@ -42,18 +42,16 @@ This will create a new blank script within the package R/ folder. This will create a new blank test script within the package testthat/ folder. -## Updating the package version - -Once changes have been completed, reviewed and are ready for use in the wild, you -can increment the package version using: - -`usethis::use_version()` - -Once you've incremented the version number, it'll add a new heading to news.md. +If you want to test the function on a live dashboard using shinytest2, we have a test_dashboard that can be found in the tests/ folder where you can add examples and tests. You can run this dashboard using: +``` +shiny::runApp("tests/test_dashboard") +``` -Add a summary under news.md and then accept it's offer to commit on your behalf. -Once pushed and on the main branch, create a new release in GitHub itself. +You can run the tests against the dashboard using: +``` +shinytest2::test_app("tests/test_dashboard") +``` ## Running tests @@ -61,11 +59,25 @@ You should run the following lines to test the package locally: ``` # To check functionality devtools::check() # Ctrl-Shft-E -shinytest2::test_app("tests/test_dashboard") # important as not currently ran in CI checks +shinytest2::test_app("tests/test_dashboard") # For code styling styler::style_pkg() lintr::lint_package() ``` -If you get a lot of lintr errors, particularly around things not being defined, make sure to load the package first using Ctrl-Shft-L or `devtools::load_all(".")`, then run again. There's a known issue with lintr not picking up on bindings until packages are loaded +If you get a lot of lintr errors, particularly around things not being defined, make sure to load the package first using Ctrl-Shft-L or `devtools::load_all(".")`, then run again. There's a known issue with lintr not picking up on bindings until packages are loaded. + + +## Updating the package version + +Once changes have been completed, reviewed and are ready for use in the wild, you +can increment the package version using: + +`usethis::use_version()` + +Once you've incremented the version number, it'll add a new heading to news.md. + +Add a summary under news.md and then accept it's offer to commit on your behalf. + +Once pushed and on the main branch, create a new release in GitHub itself. diff --git a/.gitignore b/.gitignore index 7784746..4841380 100644 --- a/.gitignore +++ b/.gitignore @@ -6,4 +6,3 @@ google-analytics.html tests/testthat/google-analytics.html docs inst/doc -www/ diff --git a/DESCRIPTION b/DESCRIPTION index d7c167a..6120708 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -1,6 +1,6 @@ Package: dfeshiny Title: DfE R-Shiny Standards -Version: 0.4.0 +Version: 0.4.1 Authors@R: c( person("Rich", "Bielby", , "richard.bielby@education.gov.uk", role = c("aut", "cre"), comment = c(ORCID = "0000-0001-9070-9969")), diff --git a/NAMESPACE b/NAMESPACE index b0e5b76..2ee1248 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -14,3 +14,4 @@ export(tidy_code) importFrom(htmltools,tagList) importFrom(htmltools,tags) importFrom(magrittr,"%>%") +importFrom(shiny,addResourcePath) diff --git a/NEWS.md b/NEWS.md index 5d6c4a0..f197e13 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,3 +1,8 @@ +# dfeshiny 0.4.1 + +* Fix bug in `external_link()` where visually hidden text was not visually +hidden and whitespace was appearing if add_warning = FALSE was set. + # dfeshiny 0.4.0 * Add new `external_link()` function and look up data for `bad_link_text`. diff --git a/R/external_link.R b/R/external_link.R index acaaf43..24d5bb3 100644 --- a/R/external_link.R +++ b/R/external_link.R @@ -1,6 +1,6 @@ #' External link #' -#' Intentionally basic wrapper for html anchor elements making it easier to +#' Intentionally basic wrapper for HTML anchor elements making it easier to #' create safe external links with standard and accessible behaviour. For more #' information on how the tag is generated, see \code{\link[htmltools]{tags}}. #' @@ -23,6 +23,11 @@ #' particularly short link text and will automatically trim any leading or #' trailing white space inputted into link_text. #' +#' If you are displaying lots of links together and want to save space by +#' avoiding repeating (opens in new tab), then you can set add_warning = FALSE +#' and add a line of text above all of the links saying something like 'The +#' following links open in a new tab'. +#' #' Related links and guidance: #' #' * [Government digital services guidelines on the use of links]( @@ -58,12 +63,48 @@ #' "R Shiny", #' add_warning = FALSE #' ) +#' +#' # This will trim and show as 'R Shiny' +#' external_link("https://shiny.posit.co/", " R Shiny") +#' +#' # Example of within text +#' shiny::tags$p( +#' "Oi, ", external_link("https://shiny.posit.co/", "R Shiny"), " is great." +#' ) +#' +#' # Example of multiple links together +#' shiny::tags$h2("Related resources") +#' shiny::tags$p("The following links open in a new tab.") +#' shiny::tags$ul( +#' shiny::tags$li( +#' external_link( +#' "https://shiny.posit.co/", +#' "R Shiny documentation", +#' add_warning = FALSE +#' ) +#' ), +#' shiny::tags$li( +#' external_link( +#' "https://www.python.org/", +#' "Python documenation", +#' add_warning = FALSE +#' ) +#' ), +#' shiny::tags$li( +#' external_link( +#' "https://nextjs.org/", +#' "Next.js documenation", +#' add_warning = FALSE +#' ) +#' ) +#' ) +#' external_link <- function(href, link_text, add_warning = TRUE) { if (!is.logical(add_warning)) { stop("add_warning must be a TRUE or FALSE value") } - # Trim whitespace as I don't trust humans not to accidentally include + # Trim white space as I don't trust humans not to accidentally include link_text <- stringr::str_trim(link_text) # Create a basic check for raw URLs @@ -117,13 +158,23 @@ external_link <- function(href, link_text, add_warning = TRUE) { htmltools::span(class = "visually-hidden", "This link opens in a new tab") } - # Create link using htmltools::tags$a - htmltools::tags$a( - hidden_span, + # Create the link object + link <- htmltools::tags$a( href = href, - link_text, + htmltools::HTML(paste0(hidden_span, link_text)), # white space hack target = "_blank", rel = "noopener noreferrer", - .noWS = "after" + .noWS = c("outside") ) + + # Attach CSS from inst/www/css/visually-hidden.css + dependency <- htmltools::htmlDependency( + name = "visually-hidden", + version = as.character(utils::packageVersion("dfeshiny")[[1]]), + src = c(href = "dfeshiny/css"), + stylesheet = "visually-hidden.css" + ) + + # Return the link with the CSS attached + return(htmltools::attachDependencies(link, dependency, append = TRUE)) } diff --git a/R/zzz.R b/R/zzz.R new file mode 100644 index 0000000..841cd46 --- /dev/null +++ b/R/zzz.R @@ -0,0 +1,12 @@ +#' Adds the content of www to dfeshiny/ +#' +#' @importFrom shiny addResourcePath +#' +#' @noRd +#' +.onLoad <- function(...) { + shiny::addResourcePath( + "dfeshiny", + system.file("www", package = "dfeshiny") + ) +} diff --git a/README.md b/README.md index 240471b..c204d74 100644 --- a/README.md +++ b/README.md @@ -29,6 +29,18 @@ Otherwise: devtools::install_github("dfe-analytical-services/dfeshiny") ``` +To update the version when you're using renv, simply: + +``` r +renv::update("dfe-analytical-services/dfeshiny") +``` + +and then snapshot to update the lockfile + +```r +renv::snapshot() +``` + ## Installing functionality in development from a branch It can be useful when developing the package in particular to trial new or updated functionality in a shiny app. To do this, you can install the required branch using (replacing `branch-name` with the name of the branch you wish to install): @@ -86,7 +98,6 @@ custom_disconnect_message( Putting this on the lines *just before* the `shinyGovstyle::header(...)` line should work well. - ## Contributing Ideas for dfeshiny should first be raised as a [GitHub diff --git a/inst/www/css/visually-hidden.css b/inst/www/css/visually-hidden.css new file mode 100644 index 0000000..d543107 --- /dev/null +++ b/inst/www/css/visually-hidden.css @@ -0,0 +1,10 @@ +/* Taken from the GOV.UK design system CSS and customised */ +.visually-hidden { + position: absolute !important; + padding: 0 !important; + overflow: hidden !important; + clip: rect(0 0 0 0) !important; + border: 0 !important; + white-space: nowrap !important; + display: none; +} diff --git a/man/external_link.Rd b/man/external_link.Rd index d5f0641..01f8ab1 100644 --- a/man/external_link.Rd +++ b/man/external_link.Rd @@ -43,6 +43,11 @@ The function will error if you end with a full stop, give a warning for particularly short link text and will automatically trim any leading or trailing white space inputted into link_text. +If you are displaying lots of links together and want to save space by +avoiding repeating (opens in new tab), then you can set add_warning = FALSE +and add a line of text above all of the links saying something like 'The +following links open in a new tab'. + Related links and guidance: \itemize{ \item \href{https://design-system.service.gov.uk/styles/links/}{Government digital services guidelines on the use of links} @@ -52,7 +57,7 @@ Related links and guidance: } } \details{ -Intentionally basic wrapper for html anchor elements making it easier to +Intentionally basic wrapper for HTML anchor elements making it easier to create safe external links with standard and accessible behaviour. For more information on how the tag is generated, see \code{\link[htmltools]{tags}}. } @@ -64,4 +69,40 @@ external_link( "R Shiny", add_warning = FALSE ) + +# This will trim and show as 'R Shiny' +external_link("https://shiny.posit.co/", " R Shiny") + +# Example of within text +shiny::tags$p( + "Oi, ", external_link("https://shiny.posit.co/", "R Shiny"), " is great." +) + +# Example of multiple links together +shiny::tags$h2("Related resources") +shiny::tags$p("The following links open in a new tab.") +shiny::tags$ul( + shiny::tags$li( + external_link( + "https://shiny.posit.co/", + "R Shiny documentation", + add_warning = FALSE + ) + ), + shiny::tags$li( + external_link( + "https://www.python.org/", + "Python documenation", + add_warning = FALSE + ) + ), + shiny::tags$li( + external_link( + "https://nextjs.org/", + "Next.js documenation", + add_warning = FALSE + ) + ) +) + } diff --git a/tests/test_dashboard/tests/testthat/test-UI-01-basic_load.R b/tests/test_dashboard/tests/testthat/test-UI-01-basic_load.R index 11db292..04d77d0 100644 --- a/tests/test_dashboard/tests/testthat/test-UI-01-basic_load.R +++ b/tests/test_dashboard/tests/testthat/test-UI-01-basic_load.R @@ -1,12 +1,7 @@ # To run the diffviewer on these tests, you need to add the path: -# Doesn't work? testthat::snapshot_review('cookie-auth/', path='tests/test_dashboard/') +# Doesn't work? testthat::snapshot_review('UI-01-basic_load/', path='tests/test_dashboard/') app <- AppDriver$new( name = "basic_load", - height = 846, - width = 1445, - load_timeout = 45 * 1000, - timeout = 20 * 1000, - wait = TRUE, expect_values_screenshot_args = FALSE ) diff --git a/tests/test_dashboard/tests/testthat/test-UI-02-cookies.R b/tests/test_dashboard/tests/testthat/test-UI-02-cookies.R index 3d83c43..c77ac83 100644 --- a/tests/test_dashboard/tests/testthat/test-UI-02-cookies.R +++ b/tests/test_dashboard/tests/testthat/test-UI-02-cookies.R @@ -1,12 +1,7 @@ # To run the diffviewer on these tests, you need to add the path: -# Doesn't work? testthat::snapshot_review('cookie-auth/', path='tests/test_dashboard/') +# Doesn't work? testthat::snapshot_review('UI-02-cookies/', path='tests/test_dashboard/') app <- AppDriver$new( name = "cookies_consent", - height = 846, - width = 1445, - load_timeout = 45 * 1000, - timeout = 20 * 1000, - wait = TRUE, expect_values_screenshot_args = FALSE ) diff --git a/tests/test_dashboard/tests/testthat/test-UI-03-external_link.R b/tests/test_dashboard/tests/testthat/test-UI-03-external_link.R new file mode 100644 index 0000000..83982a6 --- /dev/null +++ b/tests/test_dashboard/tests/testthat/test-UI-03-external_link.R @@ -0,0 +1,86 @@ +# To run the diffviewer on these tests, you need to add the path: +# Doesn't work? testthat::snapshot_review('UI-03-external_link/', path='tests/test_dashboard/') +app <- AppDriver$new( + name = "external_link", + expect_values_screenshot_args = FALSE +) + +app$wait_for_idle(50) + +all_text <- app$get_text("p") + +test_that("Link text appears with correct warnings", { + expect_gt( + grep( + "Hey R Shiny (opens in new tab) is so great we should show it off more", + all_text, + fixed = TRUE + ) |> + length(), + 0 + ) +}) + +test_that("Link text appears with hidden warning", { + expect_gt( + grep( + paste0( + "Sometimes you just want to be writing This link opens in a new tabR", + " Shiny code in a cave without distractions." + ), + all_text, + fixed = TRUE + ) |> + length(), + 0 + ) +}) + +test_that("Link text doesn't have excess whitespace", { + expect_gt( + grep( + "Hey I think the greatest thing ever is R Shiny (opens in new tab).", + all_text, + fixed = TRUE + ) |> + length(), + 0 + ) + + expect_gt( + grep( + "when writing code in This link opens in a new tabR Shiny.", + all_text, + fixed = TRUE + ) |> + length(), + 0 + ) +}) + +all_links_html <- app$get_html("a") + +test_that("Inspect HTML is as expected", { + expect_gt( + grep( + "target=\"_blank\" rel=\"noopener noreferrer\">R Shiny (opens in new tab)", + all_links_html, + fixed = TRUE + ) |> + length(), + 0 + ) + + expect_gt( + grep( + paste0( + "rel=\"noopener noreferrer\">", + "This link opens in a new tabR Shiny" + ), + all_links_html, + fixed = TRUE + ) |> + length(), + 0 + ) +}) diff --git a/tests/test_dashboard/ui.R b/tests/test_dashboard/ui.R index 9ec7925..9881106 100644 --- a/tests/test_dashboard/ui.R +++ b/tests/test_dashboard/ui.R @@ -40,6 +40,48 @@ ui <- function(input, output, session) { value = "cookies_panel_ui", "Cookies", cookies_panel_ui(google_analytics_key = ga_key) # nolint: [object_usage_linter] + ), + + ## Example text panel --------------------------------------------------- + shiny::tabPanel( + value = "text_example", + "Text example", + shiny::tags$h2("Hey, here's a heading"), + shiny::tags$p( + "Hey ", + external_link( + "https://shiny.posit.co/", + "R Shiny" + ), + " is so great we should show it off more." + ), + shiny::tags$p( + "Hey I think the greatest thing ever is ", + external_link( + "https://shiny.posit.co/", + "R Shiny" + ), + "." + ), + shiny::tags$p( + "Sometimes you just want to be in a cave without distractions", + " when writing code in ", + external_link( + "https://shiny.posit.co/", + "R Shiny", + add_warning = FALSE + ), + "." + ), + shiny::tags$p( + "Sometimes you just want to be writing ", + external_link( + "https://shiny.posit.co/", + "R Shiny", + add_warning = FALSE + ), + " code in a cave without distractions." + ) ) ) ) diff --git a/tests/test_dashboard/www/dfe_shiny_gov_style.css b/tests/test_dashboard/www/dfe_shiny_gov_style.css index b904cd5..70a4f3d 100644 --- a/tests/test_dashboard/www/dfe_shiny_gov_style.css +++ b/tests/test_dashboard/www/dfe_shiny_gov_style.css @@ -47,7 +47,7 @@ a:active { padding: 6px 6px 6px 6px; } -/* This line sets the main area width. Originally set to 66.667% in the GDS +/* This line sets the main area width. Originally set to 66.667% in the GDS template. 80% works well here to reduce white space on the right hand side.*/ .col-sm-8{ width: 80%} @@ -205,35 +205,35 @@ template. 80% works well here to reduce white space on the right hand side.*/ .small-box.bg-blue{ - background-color: #1d70b8 !important; + background-color: #1d70b8 !important; color: #ffffff; padding: 120px margin:10px; } .small-box.bg-orange{ - background-color: #F46A25 !important; + background-color: #F46A25 !important; color: #ffffff; } .small-box.bg-red{ - background-color: #f499be !important; + background-color: #f499be !important; color: #ffffff; } .small-box.bg-green{ - background-color: #00703c !important; + background-color: #00703c !important; color: #ffffff; } .small-box.bg-dark-blue{ - background-color: #12436D !important; + background-color: #12436D !important; color: #ffffff; } #ss-connect-dialog { display: none !important; } #shiny-disconnected-overlay { display: none !important; } - + #ss-overlay { background-color: #000000 !important; opacity: 0.6 !important; @@ -246,7 +246,7 @@ template. 80% works well here to reduce white space on the right hand side.*/ overflow: hidden !important; cursor: not-allowed !important; } - + #custom-disconnect-dialog { background: #000000 !important; color: #FFFFFF !important; diff --git a/tests/testthat/test-external_link.R b/tests/testthat/test-external_link.R index dc8de5e..8364579 100644 --- a/tests/testthat/test-external_link.R +++ b/tests/testthat/test-external_link.R @@ -8,11 +8,11 @@ test_that("Returns shiny.tag object", { test_that("content and URL are correctly formatted", { expect_equal(test_link$attribs$href, "https://shiny.posit.co/") - expect_true(grepl("R Shiny", test_link$children[[2]])) + expect_true(grepl("R Shiny", test_link$children[[1]])) }) test_that("New tab warning appends", { - expect_true(grepl("\\(opens in new tab\\)", test_link$children[[2]])) + expect_true(grepl("\\(opens in new tab\\)", test_link$children[[1]])) }) test_that("attributes are attached properly", { @@ -21,7 +21,7 @@ test_that("attributes are attached properly", { }) test_that("hidden text is skipped", { - expect_true(is.null(test_link$children[[1]])) + expect_false(grepl("^This link opens in a new tab", test_link$children[[1]])) }) # Rest of tests against the function ========================================== @@ -50,26 +50,51 @@ test_that("New tab warning always stays for non-visual users", { test_link_hidden <- external_link("https://shiny.posit.co/", "R Shiny", add_warning = FALSE) - expect_true( - grepl("This link opens in a new tab", test_link_hidden$children[[1]]) + expect_equal( + paste0(test_link_hidden$children[[1]]), + 'R Shiny' ) }) test_that("Surrounding whitespace shrubbery is trimmed", { expect_equal( - external_link("https://shiny.posit.co/", " R Shiny")$children[[2]], + paste0(external_link("https://shiny.posit.co/", " R Shiny")$children[[1]]), "R Shiny (opens in new tab)" ) expect_equal( - external_link("https://shiny.posit.co/", "R Shiny ")$children[[2]], + paste0(external_link("https://shiny.posit.co/", "R Shiny ")$children[[1]]), "R Shiny (opens in new tab)" ) expect_equal( - external_link("https://shiny.posit.co/", " R Shiny ")$children[[2]], + paste0(external_link("https://shiny.posit.co/", " R Shiny ")$children[[1]]), "R Shiny (opens in new tab)" ) + + expect_equal( + paste0(external_link( + "https://shiny.posit.co/", " R Shiny", + add_warning = FALSE + )$children[[1]]), + 'R Shiny' + ) + + expect_equal( + paste0(external_link( + "https://shiny.posit.co/", "R Shiny ", + add_warning = FALSE + )$children[[1]]), + 'R Shiny' + ) + + expect_equal( + paste0(external_link( + "https://shiny.posit.co/", " R Shiny ", + add_warning = FALSE + )$children[[1]]), + 'R Shiny' + ) }) test_that("Warning appears for short link text and not for long text", {