-
Notifications
You must be signed in to change notification settings - Fork 45
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 relative urls where possible and fix broken and outdated links #887
Conversation
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.
This is really really awesome, thank you for doing this!
@@ -7,7 +7,7 @@ hide_in_navigation: true | |||
|
|||
# Accessibility | |||
|
|||
This accessibility statement applies to the GDS Way at [https://gds-way.digital.cabinet-office.gov.uk/](https://gds-way.digital.cabinet-office.gov.uk/). | |||
This accessibility statement applies to the GDS Way at <https://gds-way.digital.cabinet-office.gov.uk/>. |
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.
Ooh, nice, I didn't know this was a thing!
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.
Huh, TIL (example from markdown guide), cool
Outside of the manual sources there's a few more instances of HTTPS urls that could be fixed; I tried to push a commit myself but I don't have permissions, but here's a patch (feel free to squash it or drop it as you wish):
|
As well as being easier to maintain, this makes it less likely that a user will be confused if they are running the site locally or checking a PR preview and end up on the live site (without realising).
Despite the space between the link text and the `(ACL)` in brackets this is being interpreted as a markdown link with ‘ACL` as a destination, which does not exist. Rewording the link text to include the acronym and changing the name of the reference link fix this. Also make it a relative URL, rather than including an outdated domain, and match the case of the target URL to avoid a redirect.
Now that we’re using relative URLs wherever possible, it doesn’t make sense to ignore links using the full path.
I have no idea why the markdown on this page is such a mess with so much duplication – I decided unpicking this mess is a job for another day. I did however remove the link a non existent page on `gds-way-aws-preview.cloudapps.digital` (!?) because it wasn’t referenced anywhere and so wasn’t doing anything.
Co-authored-by: Laurence de Bruxelles <[email protected]>
2aa5a5f
to
cc81e1c
Compare
@@ -8,7 +8,6 @@ | |||
.split("\0") | |||
.filter { |path| path.start_with? "source/" } | |||
.map { |path| path.delete_prefix("source/").delete_suffix(".erb").delete_suffix(".md") } | |||
.map { |path| "https://gds-way.cloudapps.digital/#{path}" } | |||
|
|||
proofer = HTMLProofer.check_directory( |
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.
Great catch, was wondering why the previous changes hadn't been picked up
Help me, I've fallen down a link-fixing rabbit hole 🐰 🕳️
This PR: