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

Improved search feature for API docs #5229

Merged
merged 14 commits into from Jan 2, 2018

Conversation

Projects
None yet
6 participants
@straight-shoota
Contributor

straight-shoota commented Nov 1, 2017

This PR greatly improves client-side search on generated HTML pages of the API docs.
It uses the program description available as JSON data from #4746 and implements a custom search algorithm in JavaScript.
The search results include types, constants, methods, macros matching the query string in one or more of the fields name, namespace, description, arguments or type.
A custom ranking algorithm sorts results based on matching quality and relevance.

The search-algorithm is actually quite simple, it uses no search index or advanced information retrieval functions. It simply traverses through all the types and tries to find matching features. This is really just fast enough.
I considered using something like lunr.js but this would have been complicated to setup to create a domain-specific search experience. With the custom algorithm it is very easy to fine tune matching and ranking behaviour to allow developers to find what they need as fast as possible.

This approach has some drawbacks with regards to fulltext search capabilities (especially concerning docstrings), but this can certainly be improved upon.

Search features:

  • prefix filter: # matches instance methods, . matches class methods and macros, :/:: matches namespace
  • search is generally case-insensitive but results with case-equality areranked higher.
  • search results list can be navigated by key (when in focus):
    • i, c, ArrowUp: move upwards
    • j, h, ArrowDown: move downwards
    • Enter: load result
    • Escape: cancel search
    • arrows, Enter and Escape also work when the search-input is in focus
  • support for URL fragment-based search query: /#q=mysearch will initialize a search at page load
  • page-wide shortkeys to start search: s or /

Preview

Online Preview

This is intended to be alongside #4755 which makes it look even better (Online Preview) though it can be merged separately.

Closes #1792

@@ -78,6 +78,7 @@ class Crystal::Doc::Generator
main_index = Main.new(raw_body, Type.new(self, @program), repository_name)
File.write File.join(@output_dir, "index.json"), main_index
File.write File.join(@output_dir, "index.jsonp"), main_index.to_jsonp

This comment has been minimized.

@MakeNowJust

MakeNowJust Nov 2, 2017

Contributor

I think search-index.js (from Rust doc) is better than index.jsonp. The extension jsonp is irregular.

This comment has been minimized.

@Sija

Sija Nov 2, 2017

Contributor

Yeah, why JSONP? Still, there might a problem with loading json files over file:///.

This comment has been minimized.

@straight-shoota

straight-shoota Nov 2, 2017

Contributor

search-index.js seems like a good idea 👍

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Nov 2, 2017

@Sija Yes, browsers can't simply load arbitrary files over file protocol. That's why we need to load the search index as a javascript file to enable search when reading the docs locally.

We could also encourage people to load the docs through a local HTTP server like serve (or at best include one directly into the docs generator) but this is always one step more complicated and the workaround with JSONP is super easy.
search-index.js can obviously be ignored when deploying to a HTTP server.

@straight-shoota straight-shoota force-pushed the straight-shoota:jm-docs-search branch from 34a51af to 4a2845e Nov 2, 2017

@RX14

This comment has been minimized.

Member

RX14 commented Nov 3, 2017

Why isn't it j/k for up/down? That's the standard vim controls...

Would be fantastic if the search handled typos by giving some weight to keys that are close physically to the one actually pressed, but thats really way too much to ask especially with the number of keyboard layouts we have.

@RX14

This comment has been minimized.

Member

RX14 commented Nov 3, 2017

Bug reports:

  • pressing / only works the first time after page load on firefox, because the second time it brings up quick search.
  • If i type too quickly that search doesn't complete before i press enter, (i.e. pressing string<enter> with no delay) it doesn't wait until the search is complete before jumping.
  • pressing enter to confirm a search doesn't clear the search box and reset the element focus.
  • somehow typing .open gets me .dlopen? that's unexpected.

There should also probably be a (x) button to clear the search box.

The small side bar is also too small (for search) on vertical monitors. I'm not sure exactly how to solve that. Perhaps a popout search, or expand the sidebar on search below a window size.

Also, it's not a true fuzzy search, i'd expect htp to get me to http, or cread to get me to Char::Reader.

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Nov 3, 2017

Thanks for the testing.

  • I copies the key bindings from api.rubyonrails.org - seems like they had it wrong and I didn't notice. Will change to j/k
  • I've only done rudimentary tests in firefox. Seems a bit odd. I noticed that it might sometimes be a focus issue.
  • Fast jumping will be fixed.
  • This is supposed to be a feature. Additionally, also the sidebar scroll viewport stays the same. I find it useful this way, so you can easily look at a different search result instead of having to start over.
@RX14

This comment has been minimized.

Member

RX14 commented Nov 3, 2017

The solution is to define a peek button p which does the same as enter but focusses the search bar back again. You could even add ctrl-p so you don't need to press the down arrow

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Nov 4, 2017

@RX14 I'm not sure, I can follow.
In which state should that peek button p be available? It can't be while you're typing. Needs a modifier like Ctrl. What's the problem with the current behaviour?

@straight-shoota straight-shoota force-pushed the straight-shoota:jm-docs-search branch 2 times, most recently from 23cb560 to 7d23a89 Nov 4, 2017

@RX14

This comment has been minimized.

Member

RX14 commented Nov 5, 2017

@straight-shoota the problem is that it's unexpected. When i'm at the docs my muscle memory is to do s<query>. If I have to evaluate the page to see exactly what element im focussed on it's just going to make me a new hundred milliseconds longer because I have to change how I search. (yes I do care about it that much)

The rule should be that after I press enter I should be exactly the same node of the search state machine as I am when the page loads. And the starting node shouldn't depend on anything, a refresh should be a completely blank slate.

For people who want your behaviour of keeping the search results to browse through them, they should have a "peek" shortcut (ctrl-p) to peek the results of a search result, but they're still at the search list so they can find another result if it's incorrect. I'll probably never use peek. 99% of the time i've arrived in the right file, or at the least i'll need to correct my search term not just move to the second result. In fact i'll rarely be moving to the second result, largely my intended result will be rank 1, or i'll be refining my search term.

@RX14

This comment has been minimized.

Member

RX14 commented Nov 5, 2017

Another thing which would greatly help for muscle memory would be making it so that pressing s highlights the results of the search box. This is a natural way to make it so that using s<query> never appends to an existing search query, it's always a brand new search. If you want to edit your existing query, use right arrow to get to the end of the query, or use left arrow to add a prefix. This is much more natural and much easier to get to the end/start of the search query than just changing the focus but not selecting the whole contents.

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Nov 5, 2017

If I understand you correctly, with "Highlight the results of the search box" you mean "select the text in the query input field"?

@RX14

This comment has been minimized.

Member

RX14 commented Nov 5, 2017

@RX14

This comment has been minimized.

Member

RX14 commented Nov 12, 2017

@straight-shoota have you had any more time to work on this?

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Nov 15, 2017

@RX14 I've implemented that the results list is not automatically reloaded on a new page and when you focus the search field, the content gets selected.

I haven't done the peek feature yet. Strg + P won't work because in a browser this is the shortcut for printing. I'm not sure if it is worth to implement at all and if so, which shortcut would be suited for this.
Maybe Shift + Enter... but probably it's not necessary at all.

Preview link has been updated.

@RX14

This comment has been minimized.

Member

RX14 commented Nov 16, 2017

@straight-shoota bug: typing result types re selects the whole entry field, then overwrites it with ult. I'm pretty sure that's unintentional.

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Nov 16, 2017

Woah, yep. That event shouldn't trigger when the input is selected.

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Nov 16, 2017

I've fixed this and added a modal window listing shortkeys (can be opened with ?).

@faustinoaq

This comment has been minimized.

Contributor

faustinoaq commented Nov 16, 2017

Awesome improvement to documentation 🎉

Would be possible to hide sidebar on small screens and add a hamburger button?

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Nov 16, 2017

@faustinoaq Yes, this is the next step after #4755

@RX14

This comment has been minimized.

Member

RX14 commented Nov 18, 2017

I think that the input box should be explicitly cleared and unfocussed at onpageshow. Firefox seems to restore the textbox content and focus when the page is refreshed.

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Nov 18, 2017

@RX14 This is actually done on purpose. Each search query is stored in session storage and the last value is retrieved on page load. This is intended to help refine a query so you don't have to type again.
Autofocus on page load is also a nice feature, because when you open a docs page, the only thing you'd want to type is a search query. This is a common feature, witnessed for example on https://api.rubyonrails.com

But I see that the combination of both can be confusing, and maybe we could remove the auto-filling but keep the focus.

@RX14

This comment has been minimized.

Member

RX14 commented Nov 18, 2017

@straight-shoota autofilling is actually OK, since s selects what's filled anyway. Changing focus on pageload isnt. It means that up/down arrows or even pgup/pgdown to scroll the page don't work after a reload, which is terrible UX.

I tend to use the keyboard much more than mouse to scroll webpages.

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Nov 18, 2017

Being mostly a mouse user I didn't put that into consideration.
I guess it comes down to mouse users having to press s or keyboard users having to press Esc 🤣 But since the former is just a nice shortcut the latter is truly a huge restriction for usability, it's certainly better not to have autofocus.
Plus, this makes it possible to keep the autofill.

@RX14

This comment has been minimized.

Member

RX14 commented Nov 21, 2017

@straight-shoota have you updated the preview site? I still get the focus on the search box.

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Nov 21, 2017

Oh, sry. Now it is updated. It seems I uploaded a wrong version, will resolve it.

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Nov 21, 2017

Fixed it, preview is now correct.

straight-shoota added some commits Nov 4, 2017

Don't keep results list when clicking on result
When a search result is opened, the new page does not load the results list and the query field is not focused (`leaveSearchScope`)
To refine or start a new search, when the query field is focused (`q` or
`/` shortcut) it's content is selected.
Add custom implemention for centered scrolling position
`scrollIntoView({behaviour: smooth, block: center})` is only an experimental DOM feature and not supported by many browsers.

@straight-shoota straight-shoota force-pushed the straight-shoota:jm-docs-search branch from 69ff27c to 9b83d32 Dec 22, 2017

@RX14

RX14 approved these changes Jan 2, 2018

@RX14 RX14 added this to the Next milestone Jan 2, 2018

@RX14 RX14 merged commit 1eeb5c0 into crystal-lang:master Jan 2, 2018

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@RX14

This comment has been minimized.

Member

RX14 commented Jan 2, 2018

🎉 🎉 🎉 I've been waiting for this forever.

Can't wait for the first official crystal release with these docs on crystal-lang.org.

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Jan 2, 2018

@RX14 The current docs could already be upgraded to a version built by a compiler including this PR.

@straight-shoota straight-shoota deleted the straight-shoota:jm-docs-search branch Jan 2, 2018

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Jan 2, 2018

In fact, I would really appreciate it if it could be updated soon. That would allow us to catch and fix errors (specifically improvements for the result ranking) which will only emerge if it is broadly used. This way we could get fixes before the next release.

@RX14

This comment has been minimized.

Member

RX14 commented Jan 2, 2018

@straight-shoota I'd be okay with this, but the rest of the core team should weigh in.

@asterite

This comment has been minimized.

Contributor

asterite commented Jan 2, 2018

Using better docs for the current version sounds good to me.

lukeasrodgers added a commit to lukeasrodgers/crystal that referenced this pull request Jan 7, 2018

Improved search feature for API docs (crystal-lang#5229)
* Implement advanced search for API docs

This greatly improves client-side search on generated HTML pages of the API docs.
It uses the program description available as JSON data and implements a custom search algorithm.
The search results include types, constants, methods, macros matching the query string in
one or more of the fields name, namespace, description, arguments or type. A custom ranking algorithm
sorts results based on matching quality and relevance.

Search features:
* prefix filter: `#` matches instance methods, `.` matches class methods
and macros, `:` matches namespace
* search is generally case-insensitive but results with case-equality are
ranked higher.
* search results list can be navigated by key (when in focus):
  * `i`, `c`, `ArrowUp`: move upwards
  * `j`, `h`, `ArrowDown`: move downwards
  * `Enter`: load result
  * `Escape`: cancel search
  * arrows, `Enter` and `Escape` also work when the search-input is
  in focus
* support for URL fragment-based search query: `/#q=mysearch` will
initialize a search at page load
* page-wide shortkeys to start search: `s` or `/`

* Add jsonp support to allow search when API docs are loaded from local file system

* Add jsonp support to allow search when API docs are loaded from local file system

* Navigator: Fix keybindings, don't jump to result while searching

* Improve result ranking based on in-string match position

* Don't keep results list when clicking on result

When a search result is opened, the new page does not load the results list and the query field is not focused (`leaveSearchScope`)
To refine or start a new search, when the query field is focused (`q` or
`/` shortcut) it's content is selected.

* Fix: don't trigger global shortkeys when input has focus

* Add usage modal

* remove autofocus of search input

* Delay navigator event after search has been performed

* Keep current selected result centered on screen

* Remove scroll position persistance (interferes with results list)

* Delay execution of search until index is loaded

* Add custom implemention for centered scrolling position

`scrollIntoView({behaviour: smooth, block: center})` is only an experimental DOM feature and not supported by many browsers.

@faustinoaq faustinoaq referenced this pull request Jun 19, 2018

Open

Create Documentation, Guides and API #3

2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment