-
Notifications
You must be signed in to change notification settings - Fork 7
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 external link text code from dfeshiny #115
Conversation
e519ba5
to
a4224bf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Cam! Looks really neat and like the individual codeowners rules too to keep track of specific changes :) only a couple of teeny typos I spotted - would it also maybe be useful to add an example to the README too to show how to use?
Happy to add examples to the readme and the run example app, though I'll wait until #118 is merged and I can rebase this branch on top of that, as my over eager code formatting with styler could make the conflicts a bit messy if I edit the run_example file in this PR too without waiting! |
* add links options into footer function * add tests for footer links * add example into example app * add extra description to footer documentation * combine package load and push to shinyapps * Add external link text code from dfeshiny (#115) * Add external link text code from dfeshiny * add codeowners * fix xlsx - xslx typo in bad link text * add plain footer example to README * update footer function - missing div class * add links options into footer function * add tests for footer links * add example into example app * add extra description to footer documentation * add plain footer example to README * update footer function - missing div class * resolve merge conflicts --------- Co-authored-by: sarahmwong <sarah.m.wong96@gmail.com>
Overview of changes
This resolves #102 by adding a new function for handling external links in dashboards. It's copied over from
dfeshiny::external_link()
.This includes adding a data set to the package and associated extra bits, for the
bad_link_text
table, which is just a single column of bad link text that the function uses to validate against. Hoping this will make it easier if anyone else needs to add data too as there'll be an example of how to do it. The list of link text is the same list we use in the explore education statistics service, so I've started a codeowners file to selfishly put myself as the owner for the link text data, that way if there's any changes I should be notified and then can update the list on EES as well! Or that's the theory, never used a code owners file before, so it's a bit of an experiment. As a default I've put Sarah and I as the owners for the repo.Once merged here, I will update dfeshiny's function to just be a blank wrapper on
shinyGovstyle::external_link()
, that way anyone using the dfeshiny version will still be able to use their code without changes, but we will only have one version of the package code to maintain in this shinyGovstyle package.PR Checklist
devtools::check()
Reviewer instructions
This went through a few rounds of questionable QA on our side, missing some glaring errors (initial PR, second PR and third PR) 😅 so in theory is pretty well tested and reviewed already. Give a general check over to see if it behaves as you'd expect.
Worth also testing if the styling attaches properly, I'm not sure if I should also be attaching the GOV.UK CSS as a dependency here too as well as the custom sr-only CSS?