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

Add external link function and bad link text examples #46

Merged
merged 7 commits into from
Sep 10, 2024

Conversation

cjrace
Copy link
Contributor

@cjrace cjrace commented Sep 10, 2024

EDITIED:

Brief overview of changes

Added a custom wrapper for htmltools::tags$a() that makes external links easier to do and enforces accessibility.

Why are these changes being made?

External links aren't generally done well by analysts, explaining how to do them is clunky and if not done correctly they can be insecure as well as inaccessible.

Detailed description of changes

  1. Added external_link() function.

In this I'm intentionally using a basic regex to check for URLs as wanted to avoid making requests every time the front end of a site using this is rendered, which would have happened if I'd used RCurl or httr's custom functions that check for a valid URL.

  1. Added bad_link_text data frame with known examples of poor link text.

  2. Reordered the reference list to be vaguely alphabetical (was also nicer to have cookies at the top instead of the mostly redundant tidy_code)

Additional information for reviewers

Added a couple of extra commits since raising that remove excess whitespace, pre-emptively bump the package version, and also fix up the way links show in the documentation.

Issue ticket number/s and link

No related issues, but does relate to some of the things flagged in the DAC audit.

@cjrace cjrace marked this pull request as ready for review September 10, 2024 09:07
@cjrace
Copy link
Contributor Author

cjrace commented Sep 10, 2024

Going to add a quick extra thing to enforce .noWS = "after" as we often end up needing that for neatness

@cjrace
Copy link
Contributor Author

cjrace commented Sep 10, 2024

@cjrace cjrace added enhancement New feature or request accessibility Issues that specific cause accessibility problems on the server labels Sep 10, 2024
@cjrace
Copy link
Contributor Author

cjrace commented Sep 10, 2024

Went deeper into the roxygen2 documentation and with some experimentation have managed to get the links from the function and data documentation working nicer

Copy link
Contributor

@rmbielby rmbielby left a comment

Choose a reason for hiding this comment

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

Few minor comments. I was looking at links on the HE LEO tab earlier so tested it out on a branch on that.

R/external_link.R Outdated Show resolved Hide resolved
R/external_link.R Show resolved Hide resolved
R/external_link.R Show resolved Hide resolved
@cjrace
Copy link
Contributor Author

cjrace commented Sep 10, 2024

Addressed all of the comments from @rmbielby, added a couple more suggestions to the bad text list from Nusrath, and used Tom's suggestion of a blanket warning for particularly short strings in the link text argument.

@cjrace cjrace merged commit 6a12622 into main Sep 10, 2024
10 checks passed
@cjrace cjrace deleted the add-external-link branch September 10, 2024 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility Issues that specific cause accessibility problems on the server enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants