Support sites deployed on paths other than "/" (by generating relative links) #291

eddgrant · 2022-01-20T10:12:58Z

What are you trying to do?

Make it possible to generate a fully working site, in which all intra-site links (i.e. links within the site, between pages, to assets etc) are generated as relative links, so that the site can be deployed at a path other than "/".

Identifying a user need

At HMRC we use the tech-docs-template to deploy multiple docs-as-code sites at different paths on a single domain e.g.

docs.tax.service.gov.uk/site-1
docs.tax.service.gov.uk/site-2
docs.tax.service.gov.uk/site-3
... etc

Without these changes the site links do not function correctly.

Is it a change or a bug fix?

Change.

What’s changed?

The changes are driven via standard Middleman configuration:

set :relative_links, true
activate :relative_assets

When enabled, the above configuration causes the following changes to happen:

The Table of Contents is generated using relative links (824116a)
The Table of Contents expands, collapsed and is highlighted using relative links (a2dde29)
The search function works using relative links (a2dde29)
The /api/pages.json endpoint returns relative links, such that the tech-docs-monitor can correctly generate page URLs when raising Slack notifications (16396e7). This requires a companion tech-docs-monitor change for which the PR is here.

Testing

We are testing these changes at HMRC at the moment and haven't yet observed anything which has broken as a result of the changes. We don't believe the changes cause any regressions when building sites in the normal way using absolute links. However we'd be grateful for any additional testing people can do.

lfdebrux · 2022-01-20T13:37:13Z

@eddgrant looks like there are some linter warnings, are you happy to fix these?

eddgrant · 2022-01-20T16:04:08Z

Sure, I'll give give that a go.

spec/table_of_contents/helpers_spec.rb

lib/assets/javascripts/_modules/search.js

eddgrant · 2022-01-20T16:52:23Z

I've just realised the spec Rake task doesn't run the Jasmine tests (which are failing). I'm not familiar with Jasmine but I'll see if I can figure out what's going on there.

lfdebrux · 2022-01-20T17:10:35Z

I've just realised the spec Rake task doesn't run the Jasmine tests (which are failing). I'm not familiar with Jasmine but I'll see if I can figure out what's going on there.

You can run them locally with bundle exec rake jasmine:ci, let me know if I can be more helpful, good luck!

eddgrant · 2022-01-21T08:27:19Z

Got it! I was using EC6 const variable declarations, which work fine in my browser, but I think Jasmine may be configured to build at a prior version to ES6 and didn't like the const declarations. Changing them to var declarations has done the trick, tests are now all passing.

lib/govuk_tech_docs/path_helpers.rb

lfdebrux · 2022-01-21T12:14:35Z

lib/assets/javascripts/_modules/search.js

@@ -65,7 +67,7 @@
      // We need JavaScript to do search, so if JS is not available the search
      // input sends the query string to Google. This JS function changes the
      // input to instead send it to the search page.
-      $searchForm.prop('action', '/search')
+      $searchForm.prop('action', pathToSiteRoot + 'search/index.html')


I think it's best if this doesn't have the index.html file name, to be consistent with other URLs. Is there a reason that you changed it?

Suggested change

$searchForm.prop('action', pathToSiteRoot + 'search/index.html')

$searchForm.prop('action', pathToSiteRoot + 'search/')

If we don't include the index.html then I think we become reliant on the web server that the site is deployed behind being configured to have a default page value of index.html. Having said that I think there are other areas where we generate links which rely on the default page behaviour, so you could argue this is incongruous with that approach. Happy to try taking it out if you'd prefer consistency here.

lib/assets/javascripts/_modules/search.js

eddgrant · 2022-02-03T16:19:05Z

I've just spotted the conflict arising between 3c629e7 (recently merged to master) and 3e8770e (my change). I'm not 100% sure yet but I think the change merged to master might be a more idiomatic as it seems to use a variable which is exposed by the middleman-search gem, rather than relying on my method of walking back to the site root.

I will do some testing to see if we can replace my change with the one already merged.

This aims to address alphagov#271 by ensuring that all TOC links are generated as relative links, rather than absolute. The desired outcome being that the site no longer makes assumptions about being deployed at "/" which in turn makes it "portable" (i.e. possible to deploy it at an arbitrary path).

…relative links.

Also improved the naming of a few methods and variables.

… sites is configured to use relative links.

…n using generating sites using relative links.

To ensure test code fragment matches the actual generated code.

…ng absolute links. This configuration setting defaults to '/' so we can rely on it to always have a value.

…result links.

eddgrant · 2022-02-03T17:39:40Z

After inspection I can now see that the 2 commits do subtly different things. One provides the relative path to the site root and the other provides the path to the search index page. I have rebased my changes to include the other change and everything seems to be working.

If I push my rebased branch is it going to destroy all of our conversation above @lfdebrux ? 🤔 If it will (and that's a problem) then I could perhaps try pulling the changes in as a merge commit instead. Let me know what you'd rather?

lfdebrux · 2022-02-03T18:16:34Z

@eddgrant thanks for looking into it! A rebase and force push won't affect the conversation on this pull request page, so feel free to go ahead. It's preferred to a merge commit anyway.

lfdebrux

I think this is almost ready to be merged <3, just one small suggested change 🙏

lfdebrux · 2022-02-04T16:52:05Z

lib/assets/javascripts/_modules/search.js

@@ -51,7 +53,7 @@
    this.downloadSearchIndex = function downloadSearchIndex () {
      updateTitle('Loading search results')
      $.ajax({
-        url: searchIndexPath,
+        url: pathToSiteRoot + (searchIndexPath.startsWith('/') ? searchIndexPath.slice(1) : searchIndexPath),


Unfortunately this means we're getting http_prefix/http_prefix here as well.

I'd suggest we remove some of the complexity here and go back to just hardcoding search.json. Then we don't need both data-search-index-path and data-path-to-site-root.

Suggested change

url: pathToSiteRoot + (searchIndexPath.startsWith('/') ? searchIndexPath.slice(1) : searchIndexPath),

url: pathToSiteRoot + '/search.json',

Aah that's a shame. Happy to revert to a hardcoded search.json, however I should note we'll be undoing the changes made in 3c629e7. Is that ok?

Yep, fine by me.

lfdebrux · 2022-02-04T16:52:57Z

lib/source/layouts/_search.erb

@@ -1,5 +1,5 @@
 <% if config[:tech_docs][:enable_search] %>
-<div class="search" data-module="search" data-search-index-path="<%= search_index_path %>">
+<div class="search" data-module="search" data-path-to-site-root="<%= path_to_site_root config, current_page.path %>" data-search-index-path="<%= search_index_path %>">


Suggested change

<div class="search" data-module="search" data-path-to-site-root="<%= path_to_site_root config, current_page.path %>" data-search-index-path="<%= search_index_path %>">

<div class="search" data-module="search" data-path-to-site-root="<%= path_to_site_root config, current_page.path %>">

…json

eddgrant · 2022-02-11T08:35:31Z

@lfdebrux sorry for the delay, it's been a busy week. I've just pushed the change reverting back to always using /search.json for the search results.

lfdebrux · 2022-02-11T10:01:15Z

@lfdebrux sorry for the delay, it's been a busy week. I've just pushed the change reverting back to always using /search.json for the search results.

No worries, thanks so much for all the time you've spent on this!

eddgrant · 2022-02-11T11:22:19Z

No worries at all, likewise thanks for all your effort reviewing it! 🙏

eddgrant · 2022-02-14T10:56:01Z

Hey @lfdebrux I forgot to ask, do you have plans to tag this as a release and publish a new rubygem? I'm interested so I can plan when to deprecate our fork and move back to using this directly.

lfdebrux · 2022-02-17T13:14:21Z

Hey @lfdebrux I forgot to ask, do you have plans to tag this as a release and publish a new rubygem? I'm interested so I can plan when to deprecate our fork and move back to using this directly.

@eddgrant I've discussed this with the other working group members, we will try and put out a release soon.

lfdebrux · 2022-03-09T11:01:43Z

This change has been released in release v3.2.0 🎉

You can now configure your Tech Docs Template (TDT) to build your documentation site to use relative links to pages and assets.

Thanks @eddgrant for contributing this feature and the associated fixes.

On non-hompage page, rails helper check this config and updated the path to include nesting. This is related to alphagov/tech-docs-gem#291 and since we deploy to root, these can be removed

This was referenced Jan 20, 2022

TRG-145: Support sites which use relative URLs. alphagov/tech-docs-monitor#37

Merged

Make docs portable by using relative URLs #271

Closed

lfdebrux reviewed Jan 20, 2022

View reviewed changes

spec/table_of_contents/helpers_spec.rb Outdated Show resolved Hide resolved

lib/assets/javascripts/_modules/search.js Outdated Show resolved Hide resolved

lib/assets/javascripts/_modules/search.js Outdated Show resolved Hide resolved

lfdebrux reviewed Jan 21, 2022

View reviewed changes

lib/govuk_tech_docs/path_helpers.rb Outdated Show resolved Hide resolved

lfdebrux reviewed Jan 21, 2022

View reviewed changes

lfdebrux reviewed Jan 24, 2022

View reviewed changes

lib/assets/javascripts/_modules/search.js Outdated Show resolved Hide resolved

eddgrant added 13 commits February 3, 2022 16:52

TRG-128: Make search function work when using generating sites using …

17b5c8a

…relative links.

TRG-128: Refactor path helpers out in to their own module.

fabb8a6

Also improved the naming of a few methods and variables.

TRG-128: Make /api/pages.json endpoint return relative links when the…

d874c33

… sites is configured to use relative links.

TRG-128: Make nav bar expanding, collapsing and highlighting work whe…

73080ca

…n using generating sites using relative links.

Fix Rubocop linting errors.

a6b0eb9

Remove TODO

1f2d71b

isOnSearchPage: Remove erroneous regular expression wildcard prefix.

749fcc5

changeSearchAction(): Don't pass already in-scope variable to function.

1c3ebc8

Replace use of const with var.

54ff817

Search: Add data-path-to-site-root attribute.

840a478

To ensure test code fragment matches the actual generated code.

Use :http_prefix when calculating the path to the site root, when usi…

7ccde80

…ng absolute links. This configuration setting defaults to '/' so we can rely on it to always have a value.

TRG-128: Ensure http_prefix is not duplicated when generating search …

babdeb7

…result links.

eddgrant force-pushed the TRG-128-for-GDS-review branch from c0c061c to babdeb7 Compare February 4, 2022 08:01

lfdebrux reviewed Feb 4, 2022

View reviewed changes

TRG-128: Revert back to always obtaining search results from /search.…

d49fef0

…json

lfdebrux approved these changes Feb 11, 2022

View reviewed changes

lfdebrux merged commit 66cc7ab into alphagov:master Feb 11, 2022

lfdebrux mentioned this pull request Feb 17, 2022

Update changelog to add new 'relative links' feature #293

Merged

lfdebrux linked an issue Feb 17, 2022 that may be closed by this pull request

Make docs portable by using relative URLs #271

Closed

This was referenced Mar 9, 2022

Release v3.2.0 #294

Merged

Enable publishing via github pages, without requiring a dedicated domain name #191

Closed

Search does not work for websites hosted below '/' (which includes all Github pages sites) alphagov/tech-docs-template#213

Closed

This was referenced Mar 9, 2022

Search: Support Github pages / http_prefix #196

Merged

Assets: Support Github pages / http_prefix #197

Merged

eddgrant deleted the TRG-128-for-GDS-review branch March 24, 2022 17:16

nacnudus mentioned this pull request Jun 11, 2023

No configuration seems to work alphagov/data-community-tech-docs#78

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support sites deployed on paths other than "/" (by generating relative links) #291

Support sites deployed on paths other than "/" (by generating relative links) #291

eddgrant commented Jan 20, 2022

lfdebrux commented Jan 20, 2022

eddgrant commented Jan 20, 2022

eddgrant commented Jan 20, 2022 •

edited

Loading

lfdebrux commented Jan 20, 2022

eddgrant commented Jan 21, 2022

lfdebrux Jan 21, 2022

eddgrant Jan 21, 2022

eddgrant commented Feb 3, 2022

eddgrant commented Feb 3, 2022

lfdebrux commented Feb 3, 2022

lfdebrux left a comment

lfdebrux Feb 4, 2022 •

edited

Loading

eddgrant Feb 7, 2022

lfdebrux Feb 7, 2022

lfdebrux Feb 4, 2022

eddgrant commented Feb 11, 2022

lfdebrux commented Feb 11, 2022

eddgrant commented Feb 11, 2022

eddgrant commented Feb 14, 2022

lfdebrux commented Feb 17, 2022

lfdebrux commented Mar 9, 2022 •

edited

Loading

	$searchForm.prop('action', pathToSiteRoot + 'search/index.html')
	$searchForm.prop('action', pathToSiteRoot + 'search/')

	url: pathToSiteRoot + (searchIndexPath.startsWith('/') ? searchIndexPath.slice(1) : searchIndexPath),
	url: pathToSiteRoot + '/search.json',

	<div class="search" data-module="search" data-path-to-site-root="<%= path_to_site_root config, current_page.path %>" data-search-index-path="<%= search_index_path %>">
	<div class="search" data-module="search" data-path-to-site-root="<%= path_to_site_root config, current_page.path %>">

Support sites deployed on paths other than "/" (by generating relative links) #291

Support sites deployed on paths other than "/" (by generating relative links) #291

Conversation

eddgrant commented Jan 20, 2022

What are you trying to do?

Identifying a user need

Is it a change or a bug fix?

What’s changed?

Testing

lfdebrux commented Jan 20, 2022

eddgrant commented Jan 20, 2022

eddgrant commented Jan 20, 2022 • edited Loading

lfdebrux commented Jan 20, 2022

eddgrant commented Jan 21, 2022

lfdebrux Jan 21, 2022

Choose a reason for hiding this comment

eddgrant Jan 21, 2022

Choose a reason for hiding this comment

eddgrant commented Feb 3, 2022

eddgrant commented Feb 3, 2022

lfdebrux commented Feb 3, 2022

lfdebrux left a comment

Choose a reason for hiding this comment

lfdebrux Feb 4, 2022 • edited Loading

Choose a reason for hiding this comment

eddgrant Feb 7, 2022

Choose a reason for hiding this comment

lfdebrux Feb 7, 2022

Choose a reason for hiding this comment

lfdebrux Feb 4, 2022

Choose a reason for hiding this comment

eddgrant commented Feb 11, 2022

lfdebrux commented Feb 11, 2022

eddgrant commented Feb 11, 2022

eddgrant commented Feb 14, 2022

lfdebrux commented Feb 17, 2022

lfdebrux commented Mar 9, 2022 • edited Loading

eddgrant commented Jan 20, 2022 •

edited

Loading

lfdebrux Feb 4, 2022 •

edited

Loading

lfdebrux commented Mar 9, 2022 •

edited

Loading