🐈 [new-site-search] Add custom site search using Lunr.js. #203

jgerigmeyer · 2017-11-22T22:22:13Z

Another attempt at fixing #28.

With no real search content yet.

* master: (25 commits) Upgrade deps. compile cleanup add subtitle supplement icon path above icon preview section review cleanup display icon path, edit path style remove tooltip code, simplify icon styles Normalize iconsPath Remove filename from icon path. compile Cleanup fonts/scale docs & previews if we choose filepath on top, keep this commit filepath - version 2 - above filename Cleanup font docs force tooltips to break stylelint adding tooltip pattern styles, edit icon layout Add example to njk examples doc Add use-cases to relevant config ...

* master: Style branding update changelog Add Herman favicon. Add attribution/links to nav footer (needs styling).

jgerigmeyer · 2017-11-22T22:24:38Z

@mirisuzanne Take a look at the results here... ready for styling if the functionality looks okay. We have more control over the functionality and markup here, so let me know if you want changes.

mirisuzanne

Looks great to me!

mirisuzanne · 2017-11-22T23:04:02Z

gulpfile.js

@@ -421,3 +423,10 @@ gulp.task('imagemin', () => {
 });

 gulp.task('minify', ['jsmin', 'svgmin', 'imagemin']);
+
+gulp.task('precompile-templates', () =>


This had some issues on occasion – I had to restart my server quite often while working.

Hm, what sort of errors?

Only time I've been able to reproduce errors is when I make multiple quick changes in a row, before compilation has time to complete.

jgerigmeyer · 2017-11-23T01:59:33Z

Right now I'm showing 15 words before and after every search result match, but showing all result matches. When you search for a common word (e.g. color), that means a lot of matches on some pages. I wonder about limiting it to only display the context for the first 3 or 4 or 5 matches or something like that?

jgerigmeyer · 2017-11-23T17:25:46Z

Okay, I think this is to a point where I'm okay reviewing/merging it. There's still one small bug in the JS search tool that I'm waiting on a fix for (see olivernn/lunr.js#320), but I expect that to come in shortly.

This branch also inspired me to add two new issues (#204, which can probably wait for after 1.0, and #205, which should probably happen ASAP).

<aside>
To see the Lunr bug, search for two words, e.g. color palette -- the second word is not properly associated with the "field" it was matched in, e.g. "title" or "contents". Right now I'm defaulting to "contents", which means if it was actually matched in the "title" then it gets highlighted in the wrong place. The highlight of nting co in the following screenshot should be highlighting Palettes in the title.

</aside>

jgerigmeyer · 2017-11-23T17:29:20Z

Sidenote: I added another branch, new-site-search-fuse (https://github.com/oddbird/sassdoc-theme-herman/tree/new-site-search-fuse), which uses the Fuse.js package instead of Lunr.js. Check it out if you're interested in seeing the difference between two search algorithms. It's much smaller in size and there are a few things I like better about it, but it's much more "fuzzy" and I can't get it to act more like a typical strict search. (There are some relevant issues on the repo that I'm following, e.g. krisk/Fuse#147). So until the results there are better, I think we stick with Lunr.

wlonk · 2017-11-27T17:01:02Z

assets/js/base.js

+    if (typeof window.deparam !== 'undefined') {
+      params = window.deparam(window.location.search.substr(1));
+    }
+    return params;


Do you want to get URL params if deparam is undefined?

¯\_(ツ)_/¯ It's kind of a theoretical question... this fn should never be called currently in cases where $.deparam isn't defined. (There are various pieces of JS that are only used by the search-results page, so only loaded on that page.)

wlonk · 2017-11-27T17:27:40Z

assets/js/base.js

+        }
+        // Add search result template to set of results
+        matches = matches.add(el);
+      }


It's not urgent, but if showResults could be decomposed into well-named things, that'd probably help. It's a bit hard to follow.

wlonk · 2017-11-27T17:28:58Z

assets/js/base.js

+    showResults(results, val);
+  };
+
+  Herman.getSearchData = function getSearchData() {


Why isn't this an anonymous function assigned to Herman.getSearchData?

Good call; no reason.

wlonk · 2017-11-27T17:30:08Z

assets/js/base.js

+    // Only fetch search data if on search results page with query term
+    if (params && params.q && hasLunr && hasNunjucks) {
+      const request = new XMLHttpRequest();
+      request.open('GET', '/search-data.json', true);


Can we not use fetch here? Is this a backwards-compatibility thing?

We're currently using the slim build of jQuery, which does not include $.ajax &c. It didn't seem worth adding for simple $.getJSON call, so I did it the old-school way.

wlonk · 2017-11-27T17:32:41Z

lib/utils/render.js

@@ -1,5 +1,6 @@
 'use strict';

+const $ = require('cheerio');


TIL about cheerio!

* master: Bump version to 1.0.0-rc.1 Remove old sourcemaps

wlonk and others added 14 commits November 17, 2017 12:03

Wire up search page

f39d95d

With no real search content yet.

WIP: testing hand-rolled search

cb572c8

Strip out hand-rolled search

ffc82bf

Use Tipue live site search

f499620

Commit compiled assets

78bb077

Review cleanup.

4a2f289

Move search input to nav.

9ac4bfd

Update changelog

fa7b1f0

Set parameters for live search

7433464

Merge branch 'master' into site-search

286c791

* master: Style branding update changelog Add Herman favicon. Add attribution/links to nav footer (needs styling).

Remove svg icon <title> attr

58cfbdc

Style search

e1cb2bd

New custom site-search using Lunr.js

0de8429

jgerigmeyer added the WIP label Nov 22, 2017

jgerigmeyer assigned mirisuzanne Nov 22, 2017

jgerigmeyer changed the title ~~[new-site-search] Add custom site search using Lunr.js.~~ 🐈 [new-site-search] Add custom site search using Lunr.js. Nov 22, 2017

Style search results

d0fa852

mirisuzanne approved these changes Nov 22, 2017

View reviewed changes

jgerigmeyer added 6 commits November 22, 2017 21:05

finish compiling templates before minifying

a53cb0a

limit search result context to first 5 matches per page

d00fd98

Fix hasFuse -> hasLunr

aec354c

Add code comments and jstests.

ac33709

gitignore dist/

09bdf53

Only load JS when needed.

74244e1

jgerigmeyer added the ready for review label Nov 23, 2017

jgerigmeyer unassigned mirisuzanne Nov 23, 2017

jgerigmeyer requested a review from davisagli November 23, 2017 16:52

jgerigmeyer requested a review from wlonk November 23, 2017 16:52

jgerigmeyer mentioned this pull request Nov 23, 2017

[site-search] Wire up search page #198

Closed

jgerigmeyer removed the WIP label Nov 23, 2017

wlonk approved these changes Nov 27, 2017

View reviewed changes

Merge branch 'master' into new-site-search

f5d72c6

* master: Bump version to 1.0.0-rc.1 Remove old sourcemaps

jgerigmeyer merged commit 1d6da3a into master Nov 28, 2017

jgerigmeyer deleted the new-site-search branch November 28, 2017 15:06

jgerigmeyer mentioned this pull request Nov 29, 2017

Add Search #28

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐈 [new-site-search] Add custom site search using Lunr.js. #203

🐈 [new-site-search] Add custom site search using Lunr.js. #203

jgerigmeyer commented Nov 22, 2017 •

edited

Loading

jgerigmeyer commented Nov 22, 2017

mirisuzanne left a comment

mirisuzanne Nov 22, 2017

jgerigmeyer Nov 23, 2017

jgerigmeyer Nov 23, 2017

jgerigmeyer commented Nov 23, 2017

jgerigmeyer commented Nov 23, 2017

jgerigmeyer commented Nov 23, 2017

wlonk Nov 27, 2017

jgerigmeyer Nov 27, 2017 •

edited

Loading

wlonk Nov 27, 2017

wlonk Nov 27, 2017

jgerigmeyer Nov 27, 2017

wlonk Nov 27, 2017

jgerigmeyer Nov 27, 2017

wlonk Nov 27, 2017

🐈 [new-site-search] Add custom site search using Lunr.js. #203

🐈 [new-site-search] Add custom site search using Lunr.js. #203

Conversation

jgerigmeyer commented Nov 22, 2017 • edited Loading

jgerigmeyer commented Nov 22, 2017

mirisuzanne 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

jgerigmeyer commented Nov 23, 2017

jgerigmeyer commented Nov 23, 2017

jgerigmeyer commented Nov 23, 2017

Choose a reason for hiding this comment

jgerigmeyer Nov 27, 2017 • 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

jgerigmeyer commented Nov 22, 2017 •

edited

Loading

jgerigmeyer Nov 27, 2017 •

edited

Loading