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

@@ -42,30 +42,42 @@ 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
 
 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.

.gitignore

@@ -6,4 +6,3 @@ google-analytics.html
 tests/testthat/google-analytics.html
 docs
 inst/doc
-www/

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", , "[email protected]", role = c("aut", "cre"),
            comment = c(ORCID = "0000-0001-9070-9969")),

NAMESPACE

@@ -14,3 +14,4 @@ export(tidy_code)
 importFrom(htmltools,tagList)
 importFrom(htmltools,tags)
 importFrom(magrittr,"%>%")
+importFrom(shiny,addResourcePath)

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

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

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

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

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;
+}

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
 )
 

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
 )