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

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

Merged
merged 14 commits into from
Feb 11, 2022
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 13 additions & 7 deletions lib/assets/javascripts/_modules/collapsible-navigation.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,25 +57,31 @@
$toggleLabel.text(setOpen ? 'Collapse ' + $heading.text() : 'Expand ' + $heading.text())
}

/**
* Returns an absolute pathname to $target by combining it with window.location.href
* @param $target The target whose pathname to return. This may be an anchor with an absolute or relative href.
* @returns {string} The absolute pathname of $target
*/
function getAbsolutePath ($target) {
return new URL($target.attr('href'), window.location.href).pathname
}

function openActiveHeading () {
var $activeElement
var currentPath = window.location.pathname
var isActiveTrail = '[href*="' + currentPath + '"]'
// Add an exception for the root page, as every href includes /
if (currentPath === '/') {
isActiveTrail = '[href="' + currentPath + window.location.hash + '"]'
}
for (var i = $topLevelItems.length - 1; i >= 0; i--) {
var $element = $($topLevelItems[i])
var $heading = $element.find('> a')
// Check if this item href matches
if ($heading.is(isActiveTrail)) {
if (getAbsolutePath($heading) === currentPath) {
$activeElement = $element
break
}
// Otherwise check the children
var $children = $element.find('li > a')
var $matchingChildren = $children.filter(isActiveTrail)
var $matchingChildren = $children.filter(function (_) {
return getAbsolutePath($(this)) === currentPath
})
if ($matchingChildren.length) {
$activeElement = $element
break
Expand Down
13 changes: 9 additions & 4 deletions lib/assets/javascripts/_modules/in-page-navigation.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,13 +70,18 @@
function highlightActiveItemInToc (fragment) {
// Navigation items for single page navigation don't necessarily include the path name, but
// navigation items for multipage navigation items do include it. This checks for either case.
var $activeTocItem = $tocItems.filter(
'[href="' + window.location.pathname + fragment + '"],[href="' + fragment + '"]'
)
var $activeTocItem = $tocItems.filter(function (_) {
var url = new URL($(this).attr('href'), window.location.href)
return url.href === window.location.href
})

// Navigation items with children don't contain fragments in their url
// Check to see if any nav items contain just the path name.
if (!$activeTocItem.get(0)) {
$activeTocItem = $tocItems.filter('[href="' + window.location.pathname + '"]')
$activeTocItem = $tocItems.filter(function (_) {
var url = new URL($(this).attr('href'), window.location.href)
return url.hash === '' && url.pathname === window.location.pathname
})
}
if ($activeTocItem.get(0)) {
$tocItems.removeClass('toc-link--in-view')
Expand Down
18 changes: 11 additions & 7 deletions lib/assets/javascripts/_modules/search.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
var results
var query
var maxSearchEntries = 20
var pathToSiteRoot
var searchIndexPath

this.start = function start ($element) {
Expand All @@ -27,6 +28,7 @@
$searchResultsTitle = $searchResultsWrapper.find('.search-results__title')
$searchHelp = $('#search-help')
searchIndexPath = $element.data('searchIndexPath')
pathToSiteRoot = $element.data('pathToSiteRoot')

changeSearchAction()
changeSearchLabel()
Expand All @@ -40,7 +42,7 @@
query = s.getQuery()
if (query) {
$searchInput.val(query)
doSearch(query)
doSearch(query, pathToSiteRoot)
doAnalytics()
document.title = query + ' - ' + document.title
}
Expand All @@ -51,7 +53,7 @@
this.downloadSearchIndex = function downloadSearchIndex () {
updateTitle('Loading search results')
$.ajax({
url: searchIndexPath,
url: pathToSiteRoot + (searchIndexPath.startsWith('/') ? searchIndexPath.slice(1) : searchIndexPath),
Copy link
Member

@lfdebrux lfdebrux Feb 4, 2022

Choose a reason for hiding this comment

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

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',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, fine by me.

cache: true,
method: 'GET',
success: function (data) {
Expand All @@ -67,7 +69,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')
Copy link
Member

Choose a reason for hiding this comment

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

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/')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

$searchForm.find('input[name="as_sitesearch"]').remove()
}

Expand All @@ -88,10 +90,10 @@
return query
}

function doSearch (query) {
function doSearch (query, pathToSiteRoot) {
s.search(query, function (r) {
results = r
renderResults(query)
renderResults(query, pathToSiteRoot)
updateTitle()
})
}
Expand Down Expand Up @@ -140,7 +142,7 @@
callback(getResults(query))
}

function renderResults (query) {
function renderResults (query, pathToSiteRoot) {
var output = ''
if (results.length === 0) {
output += '<p>Nothing found</p>'
Expand All @@ -151,7 +153,9 @@
var content = s.processContent(result.content, query)
output += '<li class="search-result">'
output += '<h3 class="search-result__title">'
output += '<a href="' + result.url + '">'
var pagePathWithoutLeadingSlash = result.url.startsWith('/') ? result.url.slice(1) : result.url
var url = pathToSiteRoot.startsWith('.') ? pathToSiteRoot + pagePathWithoutLeadingSlash : '/' + pagePathWithoutLeadingSlash
output += '<a href="' + url + '">'
output += result.title
output += '</a>'
output += '</h3>'
Expand Down
1 change: 1 addition & 0 deletions lib/govuk_tech_docs.rb
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ def self.configure(context, options = {})
context.activate :api_reference

context.helpers do
include GovukTechDocs::PathHelpers
include GovukTechDocs::TableOfContents::Helpers
include GovukTechDocs::ContributionBanner

Expand Down
6 changes: 4 additions & 2 deletions lib/govuk_tech_docs/pages.rb
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
module GovukTechDocs
class Pages
include GovukTechDocs::PathHelpers
attr_reader :sitemap

def initialize(sitemap, config)
def initialize(sitemap, config, current_page)
@sitemap = sitemap
@config = config
@current_page = current_page
end

def to_json(*_args)
Expand All @@ -18,7 +20,7 @@ def as_json
review = PageReview.new(page, @config)
{
title: page.data.title,
url: "#{@config[:tech_docs][:host]}#{page.url}",
url: get_path_to_resource(@config, page, @current_page).to_s,
review_by: review.review_by,
owner_slack: review.owner_slack,
}
Expand Down
30 changes: 30 additions & 0 deletions lib/govuk_tech_docs/path_helpers.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
module GovukTechDocs
module PathHelpers
def get_path_to_resource(config, resource, current_page)
if config[:relative_links]
resource_path_segments = resource.path.split("/").reject(&:empty?)[0..-2]
resource_file_name = resource.path.split("/")[-1]

path_to_site_root = path_to_site_root config, current_page.path
resource_path = path_to_site_root + resource_path_segments
.push(resource_file_name)
.join("/")
else
resource_path = resource.url
end
resource_path
end

def path_to_site_root(config, page_path)
if config[:relative_links]
number_of_ascents_to_site_root = page_path.to_s.split("/").reject(&:empty?)[0..-2].length
ascents = number_of_ascents_to_site_root.zero? ? ["."] : number_of_ascents_to_site_root.times.collect { ".." }
path_to_site_root = ascents.join("/").concat("/")
else
middleman_http_prefix = config[:http_prefix]
path_to_site_root = middleman_http_prefix.end_with?("/") ? middleman_http_prefix : "#{middleman_http_prefix}/"
end
path_to_site_root
end
end
end
14 changes: 7 additions & 7 deletions lib/govuk_tech_docs/table_of_contents/helpers.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
require "govuk_tech_docs/path_helpers"
require "govuk_tech_docs/table_of_contents/heading_tree_builder"
require "govuk_tech_docs/table_of_contents/heading_tree_renderer"
require "govuk_tech_docs/table_of_contents/heading_tree"
Expand All @@ -7,6 +8,8 @@
module GovukTechDocs
module TableOfContents
module Helpers
include GovukTechDocs::PathHelpers

def single_page_table_of_contents(html, url: "", max_level: nil)
output = "<ul>\n"
output += list_items_from_headings(html, url: url, max_level: max_level)
Expand Down Expand Up @@ -48,11 +51,7 @@ def render_page_tree(resources, current_page, config, current_page_html)

# Reuse the generated content for the active page
# If we generate it twice it increments the heading ids
content = if current_page.url == resource.url && current_page_html
current_page_html
else
resource.render(layout: false)
end
content = current_page.url == resource.url && current_page_html ? current_page_html : resource.render(layout: false)
# Avoid redirect pages
next if content.include? "http-equiv=refresh"

Expand All @@ -71,15 +70,16 @@ def render_page_tree(resources, current_page, config, current_page_html)
config[:http_prefix] + "/"
end

link_value = get_path_to_resource(config, resource, current_page)
if resource.children.any? && resource.url != home_url
output += %{<li><a href="#{resource.url}"><span>#{resource.data.title}</span></a>\n}
output += %{<li><a href="#{link_value}"><span>#{resource.data.title}</span></a>\n}
output += render_page_tree(resource.children, current_page, config, current_page_html)
output += "</li>\n"
else
output +=
list_items_from_headings(
content,
url: resource.url,
url: link_value,
max_level: config[:tech_docs][:max_toc_heading_level],
)
end
Expand Down
2 changes: 1 addition & 1 deletion lib/source/api/pages.json.erb
Original file line number Diff line number Diff line change
@@ -1 +1 @@
<%= GovukTechDocs::Pages.new(sitemap, config).to_json %>
<%= GovukTechDocs::Pages.new(sitemap, config, current_page).to_json %>
2 changes: 1 addition & 1 deletion lib/source/layouts/_search.erb
Original file line number Diff line number Diff line change
@@ -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 %>">
Copy link
Member

Choose a reason for hiding this comment

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

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 %>">

<form action="https://www.google.co.uk/search" method="get" role="search" class="search__form govuk-!-margin-bottom-4">
<input type="hidden" name="as_sitesearch" value="<%= config[:tech_docs][:host] %>"/>
<label class="govuk-label search__label" for="search" aria-hidden="true">
Expand Down
20 changes: 18 additions & 2 deletions spec/govuk_tech_docs/pages_spec.rb
Original file line number Diff line number Diff line change
@@ -1,17 +1,33 @@
RSpec.describe GovukTechDocs::Pages do
describe "#to_json" do
it "returns the pages as JSON" do
it "returns the pages as JSON when using absolute links" do
current_page = double(path: "/api/pages.json")
sitemap = double(resources: [
double(url: "/a.html", data: double(title: "A thing", owner_slack: "#2ndline", last_reviewed_on: Date.yesterday, review_in: "0 days")),
double(url: "/b.html", data: double(title: "B thing", owner_slack: "#2ndline", last_reviewed_on: Date.yesterday, review_in: "2 days")),
])

json = described_class.new(sitemap, tech_docs: {}).to_json
json = described_class.new(sitemap, {}, current_page).to_json

expect(JSON.parse(json)).to eql([
{ "title" => "A thing", "url" => "/a.html", "review_by" => Date.yesterday.to_s, "owner_slack" => "#2ndline" },
{ "title" => "B thing", "url" => "/b.html", "review_by" => Date.tomorrow.to_s, "owner_slack" => "#2ndline" },
])
end

it "returns the pages as JSON when using relative links" do
current_page = double(path: "/api/pages.json")
sitemap = double(resources: [
double(url: "/a.html", path: "/a.html", data: double(title: "A thing", owner_slack: "#2ndline", last_reviewed_on: Date.yesterday, review_in: "0 days")),
double(url: "/b/c.html", path: "/b/c.html", data: double(title: "B thing", owner_slack: "#2ndline", last_reviewed_on: Date.yesterday, review_in: "2 days")),
])

json = described_class.new(sitemap, { relative_links: true }, current_page).to_json

expect(JSON.parse(json)).to eql([
{ "title" => "A thing", "url" => "../a.html", "review_by" => Date.yesterday.to_s, "owner_slack" => "#2ndline" },
{ "title" => "B thing", "url" => "../b/c.html", "review_by" => Date.tomorrow.to_s, "owner_slack" => "#2ndline" },
])
end
end
end
64 changes: 64 additions & 0 deletions spec/govuk_tech_docs/path_helpers_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
RSpec.describe GovukTechDocs::PathHelpers do
include GovukTechDocs::PathHelpers

describe "#get_path_to_resource" do
it "calculates the path to a resource when using absolute links" do
resource_url = "/documentation/introduction/index.html"

config = {}
resource = double("resource", url: resource_url)

resource_path = get_path_to_resource(config, resource, nil)
expect(resource_path).to eql(resource_url)
end

it "calculates the path to a resource when using relative links" do
current_page_path = "/documentation/introduction/section-one/index.html"
url = "/documentation/introduction/index.html"

config = {
relative_links: true,
}
resource = double("resource", url: url, path: url)
current_page = double("current_page", path: current_page_path)

resource_path = get_path_to_resource(config, resource, current_page)
expect(resource_path).to eql("../../../documentation/introduction/index.html")
end
end

describe "#path_to_site_root" do
it "calculates the path from a page to the site root when using absolute links" do
page_path = "/documentation/introduction/index.html"

config = {
http_prefix: "/", # This is Middleman's default setting.
}

path_to_site_root = path_to_site_root(config, page_path)
expect(path_to_site_root).to eql("/")
end

it "calculates the path from a page to the site root when a middleman http prefix" do
page_path = "/bananas/documentation/introduction/index.html"

config = {
http_prefix: "/bananas",
}

path_to_site_root = path_to_site_root(config, page_path)
expect(path_to_site_root).to eql("/bananas/")
end

it "calculates the path from a page to the site root when using relative links" do
page_path = "/documentation/introduction/index.html"

config = {
relative_links: true,
}

path_to_site_root = path_to_site_root(config, page_path)
expect(path_to_site_root).to eql("../../")
end
end
end
2 changes: 1 addition & 1 deletion spec/javascripts/search-spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ describe('Search', function () {

beforeEach(function () {
module = new GOVUK.Modules.Search()
$element = $('<div class="search" data-module="search">' +
$element = $('<div class="search" data-module="search" data-path-to-site-root="/">' +
'<form action="https://www.google.co.uk/search" method="get" role="search">' +
'<input type="hidden" name="as_sitesearch" value="<%= config[:tech_docs][:host] %>"/>' +
'<label for="search" class="govuk-label search__label">Search (via Google)</label>' +
Expand Down
Loading