-
Notifications
You must be signed in to change notification settings - Fork 38
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
Changes from 13 commits
ae07fc2
17b5c8a
fabb8a6
d874c33
73080ca
a6b0eb9
1f2d71b
749fcc5
1c3ebc8
54ff817
840a478
7ccde80
babdeb7
d49fef0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -16,6 +16,7 @@ | |||||
var results | ||||||
var query | ||||||
var maxSearchEntries = 20 | ||||||
var pathToSiteRoot | ||||||
var searchIndexPath | ||||||
|
||||||
this.start = function start ($element) { | ||||||
|
@@ -27,6 +28,7 @@ | |||||
$searchResultsTitle = $searchResultsWrapper.find('.search-results__title') | ||||||
$searchHelp = $('#search-help') | ||||||
searchIndexPath = $element.data('searchIndexPath') | ||||||
pathToSiteRoot = $element.data('pathToSiteRoot') | ||||||
|
||||||
changeSearchAction() | ||||||
changeSearchLabel() | ||||||
|
@@ -40,7 +42,7 @@ | |||||
query = s.getQuery() | ||||||
if (query) { | ||||||
$searchInput.val(query) | ||||||
doSearch(query) | ||||||
doSearch(query, pathToSiteRoot) | ||||||
doAnalytics() | ||||||
document.title = query + ' - ' + document.title | ||||||
} | ||||||
|
@@ -51,7 +53,7 @@ | |||||
this.downloadSearchIndex = function downloadSearchIndex () { | ||||||
updateTitle('Loading search results') | ||||||
$.ajax({ | ||||||
url: searchIndexPath, | ||||||
url: pathToSiteRoot + (searchIndexPath.startsWith('/') ? searchIndexPath.slice(1) : searchIndexPath), | ||||||
cache: true, | ||||||
method: 'GET', | ||||||
success: function (data) { | ||||||
|
@@ -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') | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's best if this doesn't have the
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we don't include the |
||||||
$searchForm.find('input[name="as_sitesearch"]').remove() | ||||||
} | ||||||
|
||||||
|
@@ -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() | ||||||
}) | ||||||
} | ||||||
|
@@ -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>' | ||||||
|
@@ -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>' | ||||||
|
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 |
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 %> |
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 %>"> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
<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"> | ||||||
|
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 |
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 |
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.
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 bothdata-search-index-path
anddata-path-to-site-root
.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.
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?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.
Yep, fine by me.