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

GTD-1 Search #28

Merged
merged 29 commits into from
Jul 17, 2018
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
5792b5d
GTD-33: Proof of concept for search using middleman-search
lewisnyman Jun 18, 2018
daf694e
GTD-33: Index all pages, also include content in the json object so w…
lewisnyman Jun 18, 2018
82db1fc
GTD-33: Tidying up the search UI
lewisnyman Jun 19, 2018
61d477e
GTD-33: Added the little caret
lewisnyman Jun 19, 2018
a1d724e
GTD-33: Added close button
lewisnyman Jun 19, 2018
0d8681f
GTD-33: Added focus styling to close button
lewisnyman Jun 21, 2018
286e58c
GTD-33: Added search query highlighting in result content
lewisnyman Jun 21, 2018
e47742c
GTD-33: Added middleman-search dependency
lewisnyman Jun 21, 2018
53fe69d
GTD-33: Tidy up search results content and only present the first fiv…
lewisnyman Jun 25, 2018
7716ffd
GTD-33: Make the search results scrollable
lewisnyman Jun 25, 2018
f4da2c4
GTD-33: Improved the screen reader UX
lewisnyman Jun 26, 2018
858434d
GTD-33: Moved the search index loading indicator into the search resu…
lewisnyman Jun 26, 2018
ea7ab30
GTD-1: Don't print content if there isn't any.
lewisnyman Jun 26, 2018
6fb53da
GTD-1: If the lunr index has not loaded, wait until it has and retry
lewisnyman Jun 27, 2018
4070b38
GTD-1: Improved the search results display on narrow screen sizes
lewisnyman Jun 27, 2018
348f996
GTD-1: Lowercase results title
lewisnyman Jul 2, 2018
fb5f3bd
GTD-1: Rename searchIndex function to be more clear
lewisnyman Jul 2, 2018
f32b257
GTD-1: Linting
lewisnyman Jul 2, 2018
56141f0
GTD-1: Extract the first five sentences from a page, not the last five
lewisnyman Jul 2, 2018
e10974c
GTD-1: Add a test to ensure sentences are marked an split correctly.
lewisnyman Jul 2, 2018
2dc84bb
GTD-1: Add an option to enable search functionality
lewisnyman Jul 2, 2018
6219226
GTD-1: Move var assignments close above the lines they are used
lewisnyman Jul 4, 2018
a642146
GTD-1: Change search option to enable_search for consistency with oth…
lewisnyman Jul 4, 2018
02033a7
GTD-1: Polyfill string.includes to prevent phantomJS from erroring th…
lewisnyman Jul 4, 2018
2a00441
GTD-1: Write integration tests for search
lewisnyman Jul 4, 2018
8b32eb2
GTD-1: Add a unit test for search the lunr index and returning results.
lewisnyman Jul 4, 2018
aee8e58
Merge branch 'master' into GTD-1-search
lewisnyman Jul 4, 2018
f71c99c
GTD-1: Update label f'or 'attribute to match search input id 🤦‍
lewisnyman Jul 13, 2018
014b1f9
GTD-1: Prevent search results and close button from recieving keyboar…
lewisnyman Jul 13, 2018
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
2 changes: 2 additions & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,5 @@ source 'https://rubygems.org'

# Specify your gem's dependencies in govuk_tech_docs.gemspec
gemspec

gem 'middleman-search', :git => "git://github.com/alphagov/middleman-search.git"
Copy link
Contributor

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 require middleman-search by default so sites that don't use search don't need to download it.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

2 changes: 2 additions & 0 deletions govuk_tech_docs.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this conflict with the statement in the Gemfile?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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"
Expand Down
171 changes: 171 additions & 0 deletions lib/assets/javascripts/_modules/search.js
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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think searchIndex is a weird name for this function, since it doesn't return the search index, or do any indexing, it just does a search. Maybe getResults would be clearer.

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;
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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("|");
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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';
Copy link
Contributor

Choose a reason for hiding this comment

The 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);



1 change: 1 addition & 0 deletions lib/assets/javascripts/_start-modules.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
//= require _modules/navigation
//= require _modules/page-expiry
//= require _modules/table-of-contents
//= require _modules/search

$(document).ready(function() {
GOVUK.modules.start();
Expand Down
Loading