Skip to content
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

Initial search implementation (fullwidth-only) #626

Closed
wants to merge 2 commits into from

Conversation

esad
Copy link
Collaborator

@esad esad commented Jul 29, 2016

I wanted to get early feedback on how to best integrate the search feature (See #14) that we described in our blog post.

This PR adds support for dumping search.json index if the --searchable configuration flag is set, as well as actually searching this index (via lunr.js) and displaying the matches in the "fullwidth" theme.

I've generated Realm ObjC docs with this branch, you can see it in action here.

Couple of questions:

  • Is typeahead/lunr.js dependency fine?
  • Should we turn on --searchable per default?
  • How important is searching locally (documentation viewed via file:// URLs)? Some browsers (Chrome) don't support XHR requests from file:// URLs so we'd have to figure out a different way of downloading the index.
  • Maybe it's time to refactor the theme structure in jazzy a bit so that we don't have 2 separate copies for shared assets such as jquery, lunr etc.

esad added 2 commits June 30, 2016 13:56
…ist of all declaration names, their parent declaration name and abstracts, meant for indexing in lunr.js
@DangerCI
Copy link

        1 Error
    
  </th>
 </tr>
🚫

Please include a CHANGELOG entry. You can find it at CHANGELOG.md.

        2 Warnings
    
  </th>
 </tr>
⚠️ Big PR
⚠️ This PR may need tests.
        1 Message
    
  </th>
 </tr>
📖 Note, we hard-wrap at 80 chars and use 2 spaces after the last line.

Generated by 🚫 danger

@@ -0,0 +1,22 @@
require 'tempfile'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to be unused

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@esad: Any comments? Can we get this removed?

@orta
Copy link
Collaborator

orta commented Sep 6, 2016

Is typeahead/lunr.js dependency fine?

IMO, this is the only reasonable way, I'm 👍 with that

Should we turn on --searchable per default?

I'm for it ( I'll be doing so for CocoaDocs when I can )

How important is searching locally (documentation viewed via file:// URLs)? Some browsers (Chrome) don't support XHR requests from file:// URLs so we'd have to figure out a different way of downloading the index.

This one I'm less sure of, someone else may have a better sense, I guess the thing here is that if you're reading it via Dash or Xcode's inline reader - they have their own searches, so maybe the these can take those into account and not show the search input there?

Maybe it's time to refactor the theme structure in jazzy a bit so that we don't have 2 separate copies for shared assets such as jquery, lunr etc.

I've read issues about React-ifiying the theme, but that might be engineering effort that might be worth saving for a separate PR

@jpsim
Copy link
Collaborator

jpsim commented Jan 10, 2017

Merged in #725.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants