Autocomplete for search #51
Conversation
There was a problem hiding this comment.
This is awesome, @afischer. At a high level, I'm curious if we could serve the files listing similar to the tree data in memory rather than the HTML/redirect caching layer. What do you think?
| @@ -16,6 +16,7 @@ | |||
|
|
|||
| <p class="help-text"><%- template('footer.helpText') %></p> | |||
| </div> | |||
| <script src="https://cdnjs.cloudflare.com/ajax/libs/typeahead.js/0.11.1/typeahead.jquery.min.js"></script> | |||
There was a problem hiding this comment.
It looks like all the other Library scripts are in the head (and we just haven't audited this in a while). I know it's beyond the scope of this particular change, but I think it would be worthwhile for us to be consistent about where scripts are included. This script seems like it is closer to the right place than the existing ones. What do you think about moving the other includes so they can all be together? My hunch is that this wouldn't break anything but would be curious to see what you find.
There was a problem hiding this comment.
Yeah — I initially had it in the head but since it's technically not necessary for the page to paint I wanted to make sure it wasn't blocking. That said, CDNJS has extremely aggressive caching, so I wouldn't be too worried about putting it back
There was a problem hiding this comment.
Thanks, @afischer! This looks great, just two minor questions I'm curious to get your thoughts on.
Description of Change
Adds typeahead autocomplete to Library search using Twitter's typeahead.js.
This introduces a new external dependency, but it is being served from CDNJS just as jQuery is. The script tag can probably be moved to the footer, but since it will basically always be served from cache I've left it in the head for now.
Things to discuss
There are a few things that could use some work/discussion here...
Cache methodology
As the PR stands, the
/filename-listing.jsonendpoint is hit on page load, which will either 304 or return an updated listing (similar to the whoami endpoint). This will then be stored in the browser's localStorage. This ensures users will always have the most up to date file listing.This will introduce a large number of HEAD requests to the server, which may or may not be an issue. This could be mitigated somewhat by including a relatively short
max-ageto theCache-Control. Would be interested to hear from @isaacwhite and others.Another solution I explored was checking the timestamp of the localStorage payload and only requesting the resource if it was older than some period. This completely eliminates the request.
Design!
A design/UX pass on the typeahead CSS and functionality would be appreciated!
One thing I've considered is sending the user directly to the article page if they select a suggestion in the menu, as (in theory) it will only correspond to one document. That could get messy with filenames that are named a prefix of another document name, though.
Search functionality
Currently, the search just returns a list of substring-matched filenames. Typeahead.js allows for pretty significant customization with regards to how searching works. We could do fuzzy searching, match against lists of frequently misspelled words/synonyms, etc. We could also make the search function customizable via the current customization architecture if we wanted...
This is all probably out of scope for this PR, though.
Related Issue
Closes #6 by @maxine, though there should definitely be another isue created for the search being too picky about string matching... "night" should definitely return "knight" in the results.
Motivation and Context
As outlined in the original issue, this should make it easier to search large
Checklist
npm run lintand updated code style accordinglynpm run testpasses