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

Incorporate Algolia-based search #642

Merged
merged 3 commits into from
Feb 24, 2020
Merged

Conversation

pepopowitz
Copy link
Contributor

@pepopowitz pepopowitz commented Feb 21, 2020

Addresses #633

This PR implements a local plugin hooking up Algolia docsearch. It is based on an external plugin that didn't quite work for our scenario. (Specifically, the search box only worked on initial page render, due to a known issue in that plugin.) I followed the lead of a comment on that issue, but moved all functionality into a local plugin instead of using the external one. We were only using about 12 lines of the external version, and about 30 lines were not working for us, so it felt wrong to keep using the external one.

In action

palette-search

Things to note in the gif

  • It takes you to the prod site when clicking on a result locally, I'm assuming because the URLs from algolia include the origin. A minor annoyance that I don't think is worth finding a way around, but let me know if you feel otherwise.
  • The target URL includes #__gatsby at the end, again because it's in the Algolia URL. I'm not sure how it ends up there or how to get rid of it? I think it's probably just another minor annoyance.

Outdated information (kept here for posterity)

Original PR body

This PR adds a plugin for hooking up Algolia docsearch. Unfortunately, the plugin doesn't work very well for our scenario - it only connects the search box to Algolia on initial page render. As soon as you start clicking around the site, the search box becomes functionless. This is a known issue in that plugin. It is a result of our search box re-rendering at each new route, something the plugin doesn't account for. I dug a bit to try to fix it within the plugin so I could submit a PR, but I was unsuccessful.

Instead, I followed the lead of a comment on that issue. I'm still using the plugin to render the JS and CSS inclusions into the DOM, but connecting the actual search box with an effect in the SearchBox component.

As a result, we're only using about 12 lines of code from the plugin. In addition, we have to provide settings for the plugin to work, but they aren't actually used by the plugin (or the workaround - I don't think I can access the options for another plugin.) My preference at this point would be to ditch the plugin in favor of implementing those 12 lines ourselves, but I'd love to hear your opinions.

Remaining work on this PR:

  • See what kind of styling I can do to the search results
  • Ditch the plugin that only partially works for our scenario, in favor of implementing the few lines that do work in our own codebase. Maybe?
  • Figure out how to get the algolia settings from gatsby-config.js.

@damassi
Copy link
Member

damassi commented Feb 21, 2020

My preference at this point would be to ditch the plugin in favor of implementing those 12 lines ourselves, but I'd love to hear your opinions.

I'm down with that! We can always iterate, same with the other current annoyances. An MVP with search is already a vast improvement over what we have.

@@ -0,0 +1,30 @@
const React = require("react")

exports.onRenderBody = (
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know how to make this plugin work with ES module syntax, but it isn't something that bothers me. If it bothers anyone else, maybe we could pair on it.

Copy link
Member

Choose a reason for hiding this comment

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

ES module syntax doesn't work completely in gatsby yet. Caused a bit of stress trying to figure that out when i was working on this before because it kinda worked at times, and was confusing.

@pepopowitz pepopowitz requested a review from damassi February 24, 2020 17:24
@pepopowitz pepopowitz changed the title [WIP] Incorporate Algolia-based search Incorporate Algolia-based search Feb 24, 2020
Copy link
Member

@damassi damassi left a comment

Choose a reason for hiding this comment

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

@pepopowitz - is it possible to exclude certain pages from results? If so, can we remove the changelog page?

Otherwise, looks great 👍 Merge when ready and we can start looking more closely at how algolia indexes our pages.

@pepopowitz
Copy link
Contributor Author

pepopowitz commented Feb 24, 2020

I think we can use a stop_urls setting to exclude them, but I have to figure out how/where to actually use that setting. I'll merge this and look into exclusions as follow-up work.

@pepopowitz pepopowitz merged commit 8d24d65 into artsy:master Feb 24, 2020
@pepopowitz pepopowitz deleted the algolia-search branch February 24, 2020 20:10
@artsyit
Copy link
Contributor

artsyit commented Feb 24, 2020

🚀 PR was released in v7.1.1 🚀

@pepopowitz
Copy link
Contributor Author

FYI @damassi I believe that this PR, when merged, should remove the changelog from our search results.

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.

3 participants