GTD-1 Search #28

lewisnyman · 2018-06-26T15:37:15Z

This integrates middleman-search to generate a Lunr.js index. On the frontend it's a custom Lunr.js implementation.

You'll need a forked version of middleman-search to run this locally. To do this you'll need to add this line to the Gemfile in your tech docs folders:
gem 'middleman-search', :git => "git://github.com/alphagov/middleman-search.git"

Another issue here is that middleman-search generates an index of pages, so this functionality is only useful on multi page sites. We plan to fork middleman-search and build an index of sub headings as well as pages.

In the meantime, I'd like to get this merged as soon as possible behind a config flag, so teams can start to use it and provide feedback.

…e can display content previews

…e sentences for each result Based on feedback from Stephen

The search results pane is positioned absolute which makes it harder to manage when it overflows the viewport but placing it directly after the search form maintains a nice tabbing order

Fixed the aria-hidden state management Added the number of search results in am aria-live region Prevented non-JS Google search submission

…lts title This should be more accessible, as it's an aria-live region

lewisnyman · 2018-06-27T13:26:58Z

Here's a screenshot of how the results look on mobile:

MatMoore

This looks good to me, other than the points I commented on inline. I'm hoping to try it out on a new project during the GOV.UK firebreak next week.

Could we capture analytics about how this search functionality is being used? I.e. what people search for and what they click? Having search terms especially would be really helpful for identifying gaps in the documentation and improving it.

I understand the tech docs do already integrate with GA but I don't know the details of it. But I am familiar with how the GOV.UK site search analytics is set up and am happy to talk through that if there's anything you could reuse there.

MatMoore · 2018-06-29T08:57:06Z

lib/source/layouts/_search.erb

+  <div id="search-results" class="search-results" aria-hidden="true"> 
+    <div class="search-results__inner"> 
+      <button class="search-results__close">Close<span class="search-results__close-label"> search results</span></button>
+      <h2 class="search-results__title" aria-live="polite">Results</h2>


Does this aria-live apply to the content below?

I would have expected a div around the heading + content with this attribute, rather than it just on the heading. Also, how does it combine with the aria-hidden attribute on the parent div?

This is not my area of expertise so sorry if these are obvious questions.

This is an interesting question. Originally I had the aria-live attribute around the entire search results area, but this had two detrimental effects from my POV.

Voiceover would continuously read out "Unchanged 'Close Search Results" every time you typed. That text element never changes so it was useless noise.

On every keypress Voiceover would start reading out all the results headings and the example sentences which felt like a lot of overwhelming noise. It felt like it would be better not to have aria-live at all.

In the end I added the update to the number of search results, so screen reader users get short feedback on their search query, and can then progress and view each results in their time.

We have a booking with the accessibility clinic. I'll open a new issue if they make any suggestions.

MatMoore · 2018-06-29T09:13:53Z

lib/assets/javascripts/_modules/search.js

+      });
+    }
+
+    function searchIndex(query) {


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.

MatMoore · 2018-06-29T09:22:39Z

lib/assets/javascripts/_modules/search.js

+        output += '<li class="search-result">';
+        output += '<h3 class="search-result__title">';
+        output += '<a href="' + result.url + '">';
+        output += result.title;


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?

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?

MatMoore · 2018-06-29T09:30:49Z

lib/assets/javascripts/_modules/search.js

+      content = $(content).mark(query);
+
+      // Split content by sentence.
+      var sentences = content.html().replace(/(\.+|\:|\!|\?|\r|\n)(\"*|\'*|\)*|}*|]*)/gm, "|").split("|");


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.

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.

MatMoore · 2018-06-29T09:32:13Z

lib/assets/javascripts/_modules/search.js

+    function updateTitle(text) {
+      if(typeof text == "undefined") {
+        var count = results.length;
+        var text = count + ' Results';        


minor stylistic thing - should "results" be lowercased here?

The function has to be public to unit test it. Also, display *five* sentences...

tijmenb · 2018-07-03T09:12:51Z

spec/javascripts/search-spec.js

+  });
+
+  it('returns concatenated sentences that contain the search query', function() {
+    expect(processedContent).toEqual(expectedResults)


I was confused by this because expectedResults is used before it's set. Would it be better to move everything into the it block? I'm still not entirely sure we're not accidentally doing expect(null).toEqual(null) here.

tijmenb · 2018-07-03T09:17:48Z

govuk_tech_docs.gemspec

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


Does this conflict with the statement in the Gemfile?

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/

tijmenb · 2018-07-03T09:19:21Z

docs/configuration.md

+Enables search functionality. This indexes pages only and is not recommended for single-page sites.
+
+```yaml
+search: true


Perhaps this can be enable_search for consistency with other boolean options like show_govuk_logo?

tijmenb · 2018-07-03T09:21:36Z

Gemfile

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


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.

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.

🚨Adding this line is now a requirement to activating search.

tijmenb

This looks sensible to me, but I think it needs a few more tests of the core functionality.

…er config options

…e jasmine:ci tests

Requires an extra copy of lunr.js as we can't include from the lunr.js provided by middleman-search

lewisnyman · 2018-07-09T11:40:44Z

@MatMoore I've created an issue on our Jira board to add analytics events. It would be good to see the event labels used on the gov.uk search so we can keep them consistent. I do think there is a little complexity around what is classified a 'search term' for a search-as-you-type ui. I'd rather add this separately so we can get this PR merged as soon as possible this week.

I've added some additional tests, let me know if there's anything else I can do to get this merged.

lewisnyman · 2018-07-09T15:28:25Z

See ConvivioTeam#1. I'll change the base branch to alphagov/master once this is merged

MatMoore

@lewisnyman that's fine with me. Feel free to ping me on slack if you want to chat about this.

I'm happy with everything conceptually. I haven't looked carefully at the frontend side of things but I think Rafal is providing another review.

…d focus when they aren't visible

paroxp

LGTM - late to the party.

jonathanglassman · 2018-07-13T09:45:57Z

@tijmenb Rafal has reviewed the front end, and yourself and @MatMoore have reviewed the back end. Are you happy to approve and merge?

See: alphagov/tech-docs-gem#28

Add search functionality as per version 1.5.0 of the `tech-docs-gem` (alphagov/tech-docs-gem#28)

This is a bit of a big commit, but it should be fine since it's mostly deletions and it's all interconnected. Things that happen in this commit: - We remove the layout overrides that exist in these docs, in favour of the layouts provided by the gem. - The layouts now provide the meta tags functionality (alphagov/tech-docs-gem#11), so we can remove our own code for that. - The page review is now provided by the gem as well (alphagov/tech-docs-gem#12), so we get rid of the custom code for that too. - We turn on the search functionality from the gem which was added in alphagov/tech-docs-gem#28 and follow up PRs. After this commit, we're mostly running code from the gem instead of our own. This should make it easier to update, and makes it more likely that we'll contribute back to the gem.

lewisnyman added 15 commits June 18, 2018 17:14

GTD-33: Proof of concept for search using middleman-search

5792b5d

GTD-33: Index all pages, also include content in the json object so w…

daf694e

…e can display content previews

GTD-33: Tidying up the search UI

82db1fc

GTD-33: Added the little caret

61d477e

GTD-33: Added close button

a1d724e

GTD-33: Added focus styling to close button

0d8681f

GTD-33: Added search query highlighting in result content

286e58c

GTD-33: Added middleman-search dependency

e47742c

GTD-33: Tidy up search results content and only present the first fiv…

53fe69d

…e sentences for each result Based on feedback from Stephen

GTD-33: Make the search results scrollable

7716ffd

The search results pane is positioned absolute which makes it harder to manage when it overflows the viewport but placing it directly after the search form maintains a nice tabbing order

GTD-33: Improved the screen reader UX

f4da2c4

Fixed the aria-hidden state management Added the number of search results in am aria-live region Prevented non-JS Google search submission

GTD-33: Moved the search index loading indicator into the search resu…

858434d

…lts title This should be more accessible, as it's an aria-live region

GTD-1: Don't print content if there isn't any.

ea7ab30

GTD-1: If the lunr index has not loaded, wait until it has and retry

6fb53da

GTD-1: Improved the search results display on narrow screen sizes

4070b38

MatMoore reviewed Jun 29, 2018

View reviewed changes

lewisnyman added 6 commits July 2, 2018 14:35

GTD-1: Lowercase results title

348f996

GTD-1: Rename searchIndex function to be more clear

fb5f3bd

GTD-1: Linting

f32b257

GTD-1: Extract the first five sentences from a page, not the last five

56141f0

GTD-1: Add a test to ensure sentences are marked an split correctly.

e10974c

The function has to be public to unit test it. Also, display *five* sentences...

GTD-1: Add an option to enable search functionality

2dc84bb

lewisnyman mentioned this pull request Jul 2, 2018

GTD-1: Added enable_search option to template alphagov/tech-docs-template#156

Merged

tijmenb reviewed Jul 3, 2018

View reviewed changes

GTD-1: Move var assignments close above the lines they are used

6219226

lewisnyman added 5 commits July 4, 2018 11:08

GTD-1: Change search option to enable_search for consistency with oth…

a642146

…er config options

GTD-1: Polyfill string.includes to prevent phantomJS from erroring th…

02033a7

…e jasmine:ci tests

GTD-1: Write integration tests for search

2a00441

GTD-1: Add a unit test for search the lunr index and returning results.

8b32eb2

Requires an extra copy of lunr.js as we can't include from the lunr.js provided by middleman-search

Merge branch 'master' into GTD-1-search

aee8e58

MatMoore approved these changes Jul 10, 2018

View reviewed changes

lewisnyman added 2 commits July 13, 2018 10:11

GTD-1: Update label f'or 'attribute to match search input id 🤦‍

f71c99c

GTD-1: Prevent search results and close button from recieving keyboar…

014b1f9

…d focus when they aren't visible

paroxp approved these changes Jul 13, 2018

View reviewed changes

jonathanglassman merged commit 173e403 into alphagov:master Jul 17, 2018

lewisnyman mentioned this pull request Jul 17, 2018

Release version 1.5.0 #32

Merged

lewisnyman added a commit to ConvivioTeam/tech-docs-template that referenced this pull request Jul 18, 2018

GTD-1: Add middleman gem override to template

c7bf3d5

See: alphagov/tech-docs-gem#28

deborahchua added a commit to alphagov/gds-tech-learning-pathway that referenced this pull request Aug 20, 2018

Enable search functionality

9302d91

Add search functionality as per version 1.5.0 of the `tech-docs-gem` (alphagov/tech-docs-gem#28)

deborahchua mentioned this pull request Aug 20, 2018

Enable search functionality alphagov/gds-tech-learning-pathway#98

Merged

tijmenb mentioned this pull request Feb 7, 2019

Use search, expiry feature and layouts from gem alphagov/govuk-developer-docs#1513

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GTD-1 Search #28

GTD-1 Search #28

lewisnyman commented Jun 26, 2018 •

edited

Loading

lewisnyman commented Jun 27, 2018

MatMoore left a comment

MatMoore Jun 29, 2018

lewisnyman Jul 2, 2018

MatMoore Jun 29, 2018

MatMoore Jun 29, 2018

lewisnyman Jul 2, 2018

MatMoore Jun 29, 2018

lewisnyman Jul 2, 2018

MatMoore Jun 29, 2018

tijmenb Jul 3, 2018 •

edited

Loading

tijmenb Jul 3, 2018

lewisnyman Jul 4, 2018

tijmenb Jul 3, 2018

tijmenb Jul 3, 2018

lewisnyman Jul 4, 2018

lewisnyman Aug 1, 2018

tijmenb left a comment

lewisnyman commented Jul 9, 2018

lewisnyman commented Jul 9, 2018

MatMoore left a comment •

edited

Loading

paroxp left a comment

jonathanglassman commented Jul 13, 2018

GTD-1 Search #28

GTD-1 Search #28

Conversation

lewisnyman commented Jun 26, 2018 • edited Loading

lewisnyman commented Jun 27, 2018

MatMoore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tijmenb Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tijmenb left a comment

Choose a reason for hiding this comment

lewisnyman commented Jul 9, 2018

lewisnyman commented Jul 9, 2018

MatMoore left a comment • edited Loading

Choose a reason for hiding this comment

paroxp left a comment

Choose a reason for hiding this comment

jonathanglassman commented Jul 13, 2018

lewisnyman commented Jun 26, 2018 •

edited

Loading

tijmenb Jul 3, 2018 •

edited

Loading

MatMoore left a comment •

edited

Loading