-
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
GTD-1 Search #28
GTD-1 Search #28
Changes from 15 commits
5792b5d
daf694e
82db1fc
61d477e
a1d724e
0d8681f
286e58c
e47742c
53fe69d
7716ffd
f4da2c4
858434d
ea7ab30
6fb53da
4070b38
348f996
fb5f3bd
f32b257
56141f0
e10974c
2dc84bb
6219226
a642146
02033a7
2a00441
8b32eb2
aee8e58
f71c99c
014b1f9
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 |
---|---|---|
|
@@ -29,9 +29,11 @@ Gem::Specification.new do |spec| | |
spec.add_dependency "middleman-livereload" | ||
spec.add_dependency "middleman-sprockets", "~> 4.0.0" | ||
spec.add_dependency "middleman-syntax", "~> 3.0.0" | ||
spec.add_dependency "middleman-search" | ||
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. Does this conflict with the statement in the 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. It does not. We can't specify a git path in the spec file, but we can in the Gemfile. We don't need to add this line in the spec file but I think it's better to only use the Gemfile to override the gem location. https://yehudakatz.com/2010/12/16/clarifying-the-roles-of-the-gemspec-and-gemfile/ |
||
spec.add_dependency "nokogiri" | ||
spec.add_dependency "redcarpet", "~> 3.3.2" | ||
|
||
|
||
spec.add_development_dependency "bundler", "~> 1.15" | ||
spec.add_development_dependency "rake", "~> 10.0" | ||
spec.add_development_dependency "capybara", "~> 2.18.0" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,171 @@ | ||
//= require lunr.min | ||
//= require _vendor/jquery.mark.js | ||
(function($, Modules) { | ||
'use strict'; | ||
|
||
Modules.Search = function Search() { | ||
var $html = $('html'); | ||
var lunrIndex; | ||
var lunrData; | ||
var $searchForm; | ||
var $searchInput; | ||
var $searchResults; | ||
var $searchResultsTitle; | ||
var $searchResultsWrapper; | ||
var $searchResultsClose; | ||
var results; | ||
var maxSearchEntries = 10; | ||
|
||
this.start = function start($element) { | ||
$searchForm = $element.find('form'); | ||
$searchInput = $element.find('#search'); | ||
$searchResultsWrapper = $element.find('.search-results') | ||
$searchResults = $searchResultsWrapper.find('.search-results__content'); | ||
$searchResultsTitle = $searchResultsWrapper.find('.search-results__title'); | ||
$searchResultsClose = $searchResultsWrapper.find('.search-results__close'); | ||
downloadSearchIndex(); | ||
attach(); | ||
}; | ||
|
||
function downloadSearchIndex() { | ||
updateTitle('Loading search index') | ||
$.ajax({ | ||
url: '/search.json', | ||
cache: true, | ||
method: 'GET', | ||
success: function(data) { | ||
lunrData = data; | ||
lunrIndex = lunr.Index.load(lunrData.index); | ||
$(document).trigger('lunrIndexLoaded'); | ||
} | ||
}); | ||
} | ||
|
||
function attach() { | ||
// Search functionality on search text input | ||
$searchInput.on('input', function (e) { | ||
e.preventDefault(); | ||
var query = $(this).val(); | ||
search(query); | ||
}); | ||
|
||
// Set focus on the first search result instead of submiting the search | ||
// form to Google | ||
$searchForm.on('submit', function(e) { | ||
e.preventDefault(); | ||
$searchResults.find('.search-result__title a').first().focus(); | ||
}); | ||
|
||
// Closing the search results, move focus back to the search input | ||
$searchResultsClose.on('click', function(e) { | ||
e.preventDefault(); | ||
$searchInput.focus(); | ||
hideResults(); | ||
}); | ||
} | ||
|
||
function searchIndex(query) { | ||
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 |
||
var results = []; | ||
lunrIndex.search(query).forEach( function (item, index) { | ||
if ( index < maxSearchEntries ) { | ||
results.push(lunrData.docs[item.ref]); | ||
} | ||
}); | ||
|
||
return results; | ||
} | ||
|
||
function search(query) { | ||
if(query === '') { | ||
hideResults(); | ||
return; | ||
} | ||
showResults(); | ||
// The index has not been downloaded yet, exit early and wait. | ||
if(!lunrIndex) { | ||
$(document).on('lunrIndexLoaded', function() { | ||
search(query); | ||
}); | ||
return; | ||
} | ||
results = searchIndex(query); | ||
renderResults(query); | ||
updateTitle(); | ||
} | ||
|
||
function renderResults(query) { | ||
var output = ''; | ||
if (results.length == 0) { | ||
output += '<p>Nothing found</p>'; | ||
} | ||
output += '<ul>'; | ||
for(var index in results) { | ||
var result = results[index]; | ||
var content = processContent(result.content, query); | ||
output += '<li class="search-result">'; | ||
output += '<h3 class="search-result__title">'; | ||
output += '<a href="' + result.url + '">'; | ||
output += result.title; | ||
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. This could inject HTML into the page. It's likely that at some point it will contain characters that require HTML encoding like &<> which would break the markup of the page. Can we use a templating language instead of string concatenation? 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 started looking into potential templating systems but when writing the unit test I realised that the index we generate with lunrjs doesn't include any HTML. I'm not against adding a templating system, but I'd rather not risk delaying this merge on a potential bike shed. How about we open a new issue for that? |
||
output += '</a>'; | ||
output += '</h3>'; | ||
if(typeof content !== 'undefined') { | ||
output += '<p>' + content + '</p>'; | ||
} | ||
output += '</li>'; | ||
} | ||
output += '</ul>'; | ||
|
||
$searchResults.html( output ); | ||
} | ||
|
||
function processContent(content, query) { | ||
var output; | ||
content = '<div>'+ content + '</div>'; | ||
content = $(content).mark(query); | ||
|
||
// Split content by sentence. | ||
var sentences = content.html().replace(/(\.+|\:|\!|\?|\r|\n)(\"*|\'*|\)*|}*|]*)/gm, "|").split("|"); | ||
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. Can you create tests for this code that generates the snippets? I think I understand what this is doing, but it's not super clear what the intended behaviour is, and if we end up changing that regex (for example) I wouldn't want to break edge cases that you've already considered. I expect we may want to truncate these further since matching sentences could be quite long, but I'd rather wait and see if it's actually an issue. 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.
Yeah I can see that being an issue. It's going to be tricky to truncate without accidentally removing required context. Let's see what comes up. |
||
|
||
// Select the first five sentences that contain a <mark> | ||
var selectedSentences = []; | ||
for (var i = sentences.length - 1; i >= 0; i--) { | ||
if(selectedSentences.length >= 4) { | ||
break; | ||
} | ||
|
||
var containsMark = sentences[i].includes('mark>'); | ||
if (containsMark) { | ||
selectedSentences.push(sentences[i].trim()); | ||
} | ||
} | ||
if(selectedSentences.length > 0) { | ||
output = ' … ' + selectedSentences.join(' … ') + ' … '; | ||
} | ||
return output; | ||
} | ||
|
||
// Default text is to display the number of search results | ||
function updateTitle(text) { | ||
if(typeof text == "undefined") { | ||
var count = results.length; | ||
var text = count + ' Results'; | ||
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. minor stylistic thing - should "results" be lowercased here? |
||
} | ||
$searchResultsTitle.text(text); | ||
} | ||
|
||
function showResults() { | ||
$searchResultsWrapper.addClass('is-open') | ||
.attr('aria-hidden', 'false'); | ||
$html.addClass('has-search-results-open'); | ||
} | ||
|
||
function hideResults() { | ||
$searchResultsWrapper.removeClass('is-open') | ||
.attr('aria-hidden', 'true'); | ||
$html.removeClass('has-search-results-open'); | ||
} | ||
}; | ||
})(jQuery, window.GOVUK.Modules); | ||
|
||
|
||
|
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.
We could add instructions to add this gem to
docs/configuration.md
. We might keep even want to keep and not requiremiddleman-search
by default so sites that don't use search don't need to download it.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.
Ok, adding this line to their tech docs gemfile would be another step to activating search? It feels like something to avoid. I don't expect tech writers to understand gemfiles and what they do, it adds another step that could break the build if done incorrectly. It's your call.
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.
🚨Adding this line is now a requirement to activating search.