Implement search page #1488

NoelDeMartin · 2018-03-14T12:43:32Z

Implementation for issue #1087. The url would be https://vuejs.org/v2/search/?q={{search query}}.

I have never worked with algolia nor hexo before, so please check that I didn't miss anything. I think the styles could also be improved. And I tried to follow the coding style the best I could (like not using semicolon), but I may have missed something.

In any case, I'll be happy to fix anything that you consider so please let me know what you think :D.

chrisvfritz · 2018-03-17T01:02:39Z

@phanan Would you mind reviewing this? I think this review requires more design expertise and that's definitely an area where you excel far beyond me. 😄

phanan

@NoelDeMartin very nice implementation, thanks! I have a couple of question though:

Is it meant to be an orphan page? I see no link to get to the page (I'm not familiar with how DuckDuckGo works FWIW)
Can/should we update the URL with every search input change? Right now they're out of sync.
Is there a reason why menu and sponsor info are hidden from the page?

phanan · 2018-03-17T07:00:36Z

themes/vue/layout/search-page.ejs

+        <input
+            class="search-query"
+            v-model="search"
+            placeholder="Search VueJS"


I'd suggest just "Search" or "Search Vue.js" here.

phanan · 2018-03-17T07:02:20Z

themes/vue/layout/search-page.ejs

+
+</div>
+
+<script src="//cdn.jsdelivr.net/algoliasearch/3/algoliasearch.min.js"></script>


These should always be https:// regardless of the original site protocol.

phanan · 2018-03-17T07:04:20Z

themes/vue/layout/search-page.ejs

+var version = match ? match[1] : 'v2'
+var algolia = algoliasearch('BH4D9OD16A', '85cc3221c9f23bfbaa4e3913dd7625ea')
+var algoliaHelper = algoliasearchHelper(algolia, 'vuejs', {
+    hitsPerPage: 15,


We use 2-space indentation (both JS and HTML) for our docs, can you please check?

phanan · 2018-03-17T07:09:09Z

themes/vue/layout/search-page.ejs

+    },
+    methods: {
+        addPage() {
+            algoliaHelper.setCurrentPage(this.lastPage+1).search()


Space around +

NoelDeMartin · 2018-03-17T09:42:27Z

Ok I have applied the changes you mentioned, to answer your questions:

DuckDuckGo is a search engine with a feature called "bangs", which allows you to search directly in other websites, and since it can be configured as a default search engine for the browser, makes it very easy to search for documentation. What we want to achieve is that you would write something like "v-model !vue" in your navigation bar and it would take you directly to this search results page, without needing to visit the search engine. More info about that here: https://duckduckgo.com/bang. About this being an orphan page, I see some value here regardless of using duck duck go or not, because you can get more results in this page (it's implemented with infinite scroll), I don't know how easy it would be to add a "show more results" button on the instant search on top linking to this page. If you think it makes sense I could look into implementing that as well.
I based my implementation on this: https://symfony.com/search and it behaves as you say. Personally I don't like it because it clutters browsing history and back arrow behaviour. But I guess it's a personal preference, so I don't know, whatever you prefer.
I don't know which menu should be here, since this searches everywhere (Guide, API, etc.). Sponsor info I'm not sure where to find it in other pages (other than the ones dedicated to that), if you tell me where to find an example I could add it.

And about the https links, I did it like the search on the top is implemented now, so maybe you want to update it as well:

<!-- search -->
<link href="//cdn.jsdelivr.net/docsearch.js/1/docsearch.min.css" rel='stylesheet' type='text/css'>
<%- css('css/search') %>
<script src="//cdn.jsdelivr.net/docsearch.js/1/docsearch.min.js"></script>

phanan · 2018-03-18T17:08:31Z

@chrisvfritz what do you think about the page being orphan? I don't have a background about this, so I can't tell
Re: URL, IMO it should behave like the Symfony one
The sponsor info is in partials/sponsors_sidebar.ejs, which is included in sidebar.ejs, which in turn is automatically included if your index.md has a type attribute. Simply adding an arbitrary type won't work that well though, as such will render the whole menu tree to the left as well (which you don't want), so you'll need to play around a bit with the logic.

chrisvfritz · 2018-03-19T03:13:56Z

I think it being an orphan is fine. If people want to search while already on the site, there's already a search bar available at the top, so people might only use a dedicated search page when being directed from other services like DuckDuckGo.

…page

NoelDeMartin · 2018-03-19T09:13:31Z

Ok I have added the push history behaviour and sponsors information. I have adblock installed and seems like sponsors are treated as ads, because I wasn't seeing them anywhere. That explains why I couldn't see them anywhere other than the sponsors page.

phanan · 2018-03-19T09:34:46Z

You can (and should) totally disable adblock for the site to test it. 2018-03-19 9:13 GMT+00:00 Noel De Martin <notifications@github.com>:

…

Ok I have added the push history behaviour and sponsors information. I have adblock installed and seems like sponsors are treated as ads, because I wasn't seeing them anywhere. That explains why I couldn't see them anywhere other than the sponsors page. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#1488 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHrt0gjJUseXZfgEnyYXLVe5kB53eOMWks5tf3a-gaJpZM4SqXB6> .

NoelDeMartin · 2018-03-19T09:36:45Z

Yeah of course, I already did, I didn't think about it before but now that I noticed I have it whitelisted.

phanan · 2018-03-19T09:40:26Z

LGTM now, thanks a lot! If we could have arrow-key navigation for the results, it would be even better, but I believe this is good enough for now.

phanan · 2018-03-19T09:42:22Z

themes/vue/source/css/_search-page.styl

+      & + .breadcrumb::before
+        content: "\203A\A0"
+        margin-left: 5px
+        color: $light


Oh, we need an EOL here btw.

NoelDeMartin · 2018-03-19T09:46:58Z

Cool, thanks :D. Yeah I think arrow navigation and adding a "view more results" link in top bar search would be good improvements in the future.

dear-lizhihua · 2018-03-19T10:09:47Z

nice page!

chrisvfritz · 2018-03-19T13:17:22Z

One quick note: if I press Enter in the search input, it currently refreshes the page and clears the search. Could we instead have it match the behavior of the top search bar, where Enter takes you to the first result?

NoelDeMartin · 2018-03-19T14:06:03Z

@chrisvfritz Yeah that's a good idea, and easy to implement. You can test in on the last commit.

chrisvfritz · 2018-03-19T15:05:54Z

Fantastic! A few other notes:

I noticed that this page uses a layout that assumes a sidebar, making it look a little strange at many screen widths because it's not quite centered. We might not need to use a completely different layout to fix this though. We have a media query to fall back to a centered layout (I think at 900px), so we could possibly use those styles for every screen width for this page.
Perhaps a nitpick, but "Search Vue.js" sounds a little ambiguous to me, like it might be searching the source code rather than this site. Maybe we should change it to just "Search" or "Search this site"?

Akryum · 2018-03-19T15:12:15Z

Preview url: https://deploy-preview-1488--vuejs.netlify.com/v2/search/

Akryum

The page is missing the 'search by Algolia' logo which is mandatory.

NoelDeMartin · 2018-03-19T15:14:07Z

@chrisvfritz Yeah, the sidebar layout looks weird, but I needed to add it because the sponsors section is located inside of the sidebar. The thing is that it has a fixed position, so it could technically be outside of the sidebar, but on mobile it is inside of the menu so I'm not sure how to go about it, I guess someone who's worked more with the site can have a better opinion. I also removed what appears by default in the sidebar, because it was a menu with only one section so I thought it'd be better to show nothing instead.

I can change the text on the search box once it's decided 100%, because I already changed it once. I still vote for using "Search VueJS" which was my initial proposal, but I don't mind which one you want to put either way.

@Akryum I didn't know it was mandatory, I'll do it in then.

NoelDeMartin

Added "search by algolia". In the symfony page seems like they have a custom url for that: https://www.algolia.com/cc/symfony?hi=symfony&utm_source=symfony&utm_medium=link&utm_campaign=symfony_search but I couldn't find one for vue so I left algolia homepage for now.

NoelDeMartin · 2018-03-19T09:49:23Z

themes/vue/source/css/_search-page.styl

+      & + .breadcrumb::before
+        content: "\203A\A0"
+        margin-left: 5px
+        color: $light


NoelDeMartin · 2018-05-02T09:57:08Z

@chrisvfritz Is there anything else needed for this to be merged?

chrisvfritz · 2018-05-02T13:07:36Z

Nope, thanks for the ping. This just fell off my radar. 🙂

phanan self-requested a review March 17, 2018 06:44

phanan reviewed Mar 17, 2018

View reviewed changes

NoelDeMartin added 3 commits March 19, 2018 10:09

vuejs#1087 Implement search page

724b23a

Fix spacing and add https in search page

6ddf3fb

Add sponsors and push history state when search is updated in search …

09a349e

…page

NoelDeMartin force-pushed the search-page branch from 4c7195c to 09a349e Compare March 19, 2018 09:11

phanan approved these changes Mar 19, 2018

View reviewed changes

phanan reviewed Mar 19, 2018

View reviewed changes

Add EOL to search styles

e1bc66d

Visit first result when submitting search form

aec4bf6

Akryum requested changes Mar 19, 2018

View reviewed changes

Add 'search by algolia' to search page

0f8420d

NoelDeMartin commented Mar 19, 2018

View reviewed changes

Akryum approved these changes Mar 19, 2018

View reviewed changes

chrisvfritz merged commit 7564a9c into vuejs:master May 2, 2018

NoelDeMartin mentioned this pull request Jan 5, 2020

Add dedicated search page cypress-io/cypress-documentation#2351

Closed

This was referenced Jul 28, 2020

Search page vuejs/docs#352

Closed

Implement search page vuejs/docs#363

Merged


		</div>

		<script src="//cdn.jsdelivr.net/algoliasearch/3/algoliasearch.min.js"></script>

Implement search page #1488

Implement search page #1488

Uh oh!

Conversation

NoelDeMartin commented Mar 14, 2018

Uh oh!

chrisvfritz commented Mar 17, 2018

Uh oh!

phanan left a comment

Choose a reason for hiding this comment

Uh oh!

phanan Mar 17, 2018

Choose a reason for hiding this comment

Uh oh!

phanan Mar 17, 2018

Choose a reason for hiding this comment

Uh oh!

phanan Mar 17, 2018

Choose a reason for hiding this comment

Uh oh!

phanan Mar 17, 2018

Choose a reason for hiding this comment

Uh oh!

NoelDeMartin commented Mar 17, 2018

Uh oh!

phanan commented Mar 18, 2018

Uh oh!

chrisvfritz commented Mar 19, 2018

Uh oh!

NoelDeMartin commented Mar 19, 2018

Uh oh!

phanan commented Mar 19, 2018 via email

Uh oh!

NoelDeMartin commented Mar 19, 2018

Uh oh!

phanan commented Mar 19, 2018

Uh oh!

phanan Mar 19, 2018

Choose a reason for hiding this comment

Uh oh!

NoelDeMartin Mar 19, 2018

Choose a reason for hiding this comment

Uh oh!

NoelDeMartin commented Mar 19, 2018

Uh oh!

dear-lizhihua commented Mar 19, 2018

Uh oh!

chrisvfritz commented Mar 19, 2018

Uh oh!

NoelDeMartin commented Mar 19, 2018

Uh oh!

chrisvfritz commented Mar 19, 2018

Uh oh!

Akryum commented Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Akryum left a comment

Choose a reason for hiding this comment

Uh oh!

NoelDeMartin commented Mar 19, 2018

Uh oh!

NoelDeMartin left a comment

Choose a reason for hiding this comment

Uh oh!

NoelDeMartin Mar 19, 2018

Choose a reason for hiding this comment

Uh oh!

NoelDeMartin commented May 2, 2018

Uh oh!

chrisvfritz commented May 2, 2018

Uh oh!

Uh oh!

Akryum commented Mar 19, 2018 •

edited

Loading