Skip to content
This repository has been archived by the owner on Dec 8, 2017. It is now read-only.

add client-side search #80

Merged
merged 32 commits into from
Feb 5, 2015
Merged

add client-side search #80

merged 32 commits into from
Feb 5, 2015

Conversation

afeld
Copy link
Contributor

@afeld afeld commented Jan 26, 2015

Closes #42.

This is a superset of #69, but the generator has been moved to the jekyll_pages_api gem, and there is now client-side code to do searches, written in Angular (which was a fairly arbitrary choice).

demo

Note that you can ignore anything under assets/components/, as those are all just vendored files.

@afeld
Copy link
Contributor Author

afeld commented Jan 26, 2015

A couple other things to note:

  • Keyboard shortcuts are supported.
    • / to jump to the search box
    • UP/DOWN arrows then ENTER to navigate to a page
  • I spent a lot more time on the search interaction than the quality of the results
    • More pages are included than we probably want, e.g. the 404 page. We can make a blacklist, but not sure if there's a more clever way to filter them.
    • Posts aren't included – see include the Posts? jekyll_pages_api#2.
    • The boosts (scoring) could probably use tweaking.

This was referenced Jan 26, 2015
mbland added a commit that referenced this pull request Jan 27, 2015
@afeld
Copy link
Contributor Author

afeld commented Jan 28, 2015

Ok, merged master and removed the unnecessary files – should be much easier to grok now!

@afeld
Copy link
Contributor Author

afeld commented Jan 28, 2015

Also, I normally don't advocate for rebasing/squashing commits once they're in a PR, but would understand if that were preferable in this case to remove the (essentially) duplicate commits from the split out PRs... let me know.

@mbland
Copy link
Contributor

mbland commented Jan 28, 2015

Yeah, go ahead and squash and rebase and push -f as makes sense.

@@ -0,0 +1,36 @@
// https://github.com/mauriciogentile/angular-livesearch/blob/1f4e357ab3b50701eeedcfb45c12cd901d612087/example/styles.css#L31-L65
ul.searchresultspopup {
Copy link

Choose a reason for hiding this comment

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

Why do you need the ul here? Why is .searchresultspopup not specific enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly, I copied-and-pasted this file from the URL above – didn't really spend any time cleaning it up. Worth a pass to delete anything we don't need, I gueeeeesssss 😑

@afeld afeld mentioned this pull request Jan 31, 2015
@mbland
Copy link
Contributor

mbland commented Feb 5, 2015

Gonna merge now, but opening an issue to address @msecret's comments and the observation that the search box appears to only allow up to nine characters.

mbland added a commit that referenced this pull request Feb 5, 2015
@mbland mbland merged commit ae2a72b into master Feb 5, 2015
@mbland mbland deleted the lunr-search branch February 5, 2015 20:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants