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

Algolia DocSearch Integration #201

Merged
merged 19 commits into from Apr 23, 2018
Merged

Algolia DocSearch Integration #201

merged 19 commits into from Apr 23, 2018

Conversation

ulivz
Copy link
Member

@ulivz ulivz commented Apr 21, 2018

  1. Try to fit the style from vue.js official website.
  2. Need to clean config and add document before merged.

A point to be considered: Since the latest docsearch.js render the distinct default style, so there are many css resets to make it looks good.

Preview: https://deploy-preview-201--vuepress.netlify.com/

Basically completed, welcome to test and feel free to give your comments.

@ulivz ulivz requested a review from yyx990803 April 21, 2018 20:42
@ulivz ulivz changed the title Algolia DocSearch Integration [WIP] Algolia DocSearch Integration Apr 21, 2018
@ulivz ulivz changed the title [WIP] Algolia DocSearch Integration Algolia DocSearch Integration Apr 21, 2018
@@ -28,6 +28,12 @@ module.exports = {
repo: 'vuejs/vuepress',
editLinks: true,
docsDir: 'docs',
algolia: {
appId: 'BH4D9OD16A',
Copy link
Member

Choose a reason for hiding this comment

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

Should the appId and apiKey be committed as part of the repo?

Copy link
Member Author

@ulivz ulivz Apr 22, 2018

Choose a reason for hiding this comment

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

Thanks for your concern, but I have commented at this PR's summary:

Need to clean config ... before merged.

@ulivz ulivz mentioned this pull request Apr 22, 2018
@ulivz
Copy link
Member Author

ulivz commented Apr 23, 2018

Still have some consideration for this feature, only one algolia option wouldn't meet the requirement, since we still want to implement locale-scoped search for that.

@ulivz
Copy link
Member Author

ulivz commented Apr 23, 2018

Seeing so many css reset, I decided to fallback to docSearch@1.5.0 that the official website uses.

@yyx990803
Copy link
Member

LGTM. Let's remove the config.

@yyx990803
Copy link
Member

Oh nvm, if you are still working on it please let me know when you finish.

@yyx990803
Copy link
Member

I think we should use the latest version of docsearch if possible. CSS resets are not that much of a problem.

@yyx990803
Copy link
Member

yyx990803 commented Apr 23, 2018

BTW we need to limit the max width of the search box on mobile (the built-in uses calc(100vw - 4rem):

screen shot 2018-04-23 at 11 37 52 am

@ulivz
Copy link
Member Author

ulivz commented Apr 23, 2018

OK, please wait for a moment. I need to limit the width of the mobile phone and test again.

@ulivz
Copy link
Member Author

ulivz commented Apr 23, 2018

@yyx990803 finished for it. now it looks good at mobile.

Another problem, I found that our search box displayed not very well at ipad's portrait view, I will fix it at another story.

@yyx990803
Copy link
Member

Can you add documentation for the algolila option please

@ulivz
Copy link
Member Author

ulivz commented Apr 23, 2018

@yyx990803

@@ -12,7 +12,7 @@
</span>
</router-link>
<div class="links">
<AlgoliaSearchBox v-if="isAlgoliaSearch" :options="$site.themeConfig.algolia"/>
<AlgoliaSearchBox v-if="isAlgoliaSearch" :options="algolia"/>
<SearchBox v-if="isSearch"/>
Copy link
Member

Choose a reason for hiding this comment

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

Can be v-else-if="$site.themeConfig.search !== false" here

- 类型: `Object`
- 默认值: `undefined`

使用 `algolia` 选项可以让你禁用掉默认的基于 headers 的搜索,从而使用 [algolia docsearch](https://github.com/algolia/docsearch)。为了使其生效,你必须提供至少 `apiKey` 和 `indexName` 这两个选项:
Copy link
Member

Choose a reason for hiding this comment

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

...可以让你用 algolia search 取代默认的...

}
```

其他可用的选项可以参考 [docsearch options](https://github.com/algolia/docsearch#docsearch-options)。
Copy link
Member

Choose a reason for hiding this comment

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

"请"参考

@@ -12,19 +12,33 @@
</span>
</router-link>
<div class="links">
<SearchBox v-if="$site.themeConfig.search !== false"/>
<AlgoliaSearchBox v-if="isAlgoliaSearch" :options="algolia"/>
<SearchBox v-if="isSearch"/>
Copy link
Member

Choose a reason for hiding this comment

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

可以用 v-else-if="$site.themeConfig.search !== false"

initialize () {
Promise.all([
import(/* webpackChunkName: "docsearch" */ 'docsearch.js/dist/cdn/docsearch.min.js'),
import(/* webpackChunkName: "docsearch" */ 'docsearch.js/dist/cdn/docsearch.min.css')
Copy link
Member

Choose a reason for hiding this comment

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

现在的 webpack 配置会把所有 css 都抽取出来,即使用 dynamic import 也没用。这样的话不论用不用 algolia 都会加载这些 css。有一个办法是在 createBaseConfig 的时候如果用户没有使用 algolia 则直接把 AlgoliaSearchBox alias 到一个空 module, 这样可以彻底避免把这个组件打包进去。

Copy link
Member Author

Choose a reason for hiding this comment

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

厉害了,我还没注意这个

Copy link
Contributor

Choose a reason for hiding this comment

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

哈哈哈哈哈,突然说中文 XD
莫名的可爱(●'◡'●)

@ulivz
Copy link
Member Author

ulivz commented Apr 23, 2018

Thanks for informing me of this detail, addressed all comments at 88f7435

And a simple size test:

  • Before:

image

  • After:

image

A great promotion! thank you!

@yyx990803 yyx990803 merged commit 2f0da01 into vuejs:master Apr 23, 2018
@jerome2710
Copy link

It looks like appId is also required, otherwise it will fallback on the default appId from the docsearch.js NPM-package which throws an error. Could this be added to the documentation as well please?

Also, could some description be added on how the content is inserted into Algolia? I am not getting any search results right now, do I need to add records to the index myself?

@bencodezen
Copy link
Member

@jerome2710 Have you submitted your site to Algolia for them to begin indexing your content?

@jerome2710
Copy link

Hi @bencodezen, thank you for your reply. No, I was unaware Algolia needed to scrape the docs-domain to index the content. I just submitted the site! Maybe add this to the documentation as well?

bencodezen pushed a commit to bencodezen/vuepress that referenced this pull request Jul 6, 2018
@bencodezen
Copy link
Member

bencodezen commented Jul 6, 2018

@jerome2710 Here's the PR for the docs update: 6082bc1 Let me know if the language isn't clear enough!

@jerome2710
Copy link

@bencodezen Very useful, thank you! Should we mention the appId here as well?

@bencodezen
Copy link
Member

bencodezen commented Jul 7, 2018

@jerome2710 I'm currently using Algolia for VueMeetups.org https://github.com/VueMeetups/vuemeetups.org and did not configure an appId. Could you send me a gist or something to show what you're talking about?

@jerome2710
Copy link

@bencodezen Interesting to hear that it does work for you. I am using v0.10.2 of Vuepress and that includes the Algolia npm-package of docsearch.js with v2.5.2. As you can see here, the constructor falls back to an default appId. This causes an appId/apiKey mismatch for me if I omit it in the config.js of Vuepress.

@ulivz ulivz mentioned this pull request Jul 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants