search

18F / jekyll_pages_api_search

DEPRECATED - Jekyll search plugin based on lunr.js and jekyll_pages_api
https://rubygems.org/gems/jekyll_pages_api_search
Other
39 stars 8 forks source link

Genericize search result rendering #24

Closed mbland closed 8 years ago

mbland commented 8 years ago

This PR refactors the original assets/js/search.js to make it more modular and introduces mechanisms for making its behavior easier to customize for users. The README.md file contains instructions that hopefully make everything clear.

Really not feeling good about the lack of JavaScript tests here; will look into adding those soon. In the meanwhile, apps.gov is already depending on this behavior. Confirmed that things continue to work for the Guides Template as well.

Happy to break this into several PRs if this is too much at once.

cc: @stroupaloop @afeld @catherinedevlin @jbarnicle @andrewmaier

jbarnicle commented 8 years ago

@mbland This all makes sense, but I'm wondering if this can't be abstracted a bit more. For example - I'm currently working on implementing search using lunr-server and I'm trying to see how to implement it using this. A couple of things that are notably different:

Having the ability to templatize the rendering of the search results would alleviate some of the tediousness of rendering complex results using pure JS.

I'm wondering if it might make sense to consolidate both approaches. Build the base functionality on lunr-server. Update lunr server so allow the ability specify which corpora to search - so that searching the hub's search-index.json would be equivalent to querying lunr-server with an additional parameter of corpora=hub (or something like that).

Reviewing this some more to make sure I completely understand before I merge. Also want to make sure others have had a chance to review (cc: @mtorres253 ).

mbland commented 8 years ago

Yeah, that's actually the direction I'd like to see this go. This PR represents another baby step in my evolution towards learning browser-side JavaScript. I agree that having some kind of template rendering may be preferable, but just haven't gotten around to learning how to do that yet.

If you want, I can try to send PRs to migrate the Node.js code from this gem into lunr-server, which the gem can then depend upon. Near-term, though, it'd be nice to get this PR in as a refactoring of the existing code and cut a new release, so apps.gov can depend on it cleanly before @stroupaloop demos it Sunday at SXSW. (The alternative is he continues to hack it based on my repo, or I cut a special gem of my own, jekyll_pages_api_search_mbland or some such, as an interim measure.)

I'm also finally making baby steps towards learning how to test this stuff in the add-tests branch of mbland/jekyll_pages_api_search, using Mocha and Chai with browserify, http-server, and PhantomJS (via mocha-phantomjs). I can try to fold those tests into this PR (if I finish it today; going to the NationJS conference), or send a new one when it's ready.

jbarnicle commented 8 years ago

@mbland - I think we can merge this as long as noone else sees any issues. As far as ongoing work, it would help if we created issues to track further changes to ensure we're not duplicating efforts, etc - as well as make sure we're on the same page in terms of direction. Making issues very granular will help keep PRs smaller (this one wasn't too bad) and make future merges a bit easier (at least on my part - still getting up to speed on a lot of this).

Thanks for the work ;-)