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.

Sign up for GitHub

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

Jump to bottom

feat: Added search capability via lunr (fixes #163) #165

Merged
merged 20 commits into from
May 21, 2020
Merged

feat: Added search capability via lunr (fixes #163) #165

merged 20 commits into from
May 21, 2020

Conversation

palant
Copy link
Collaborator

@palant palant commented May 19, 2020 •
edited
Loading

I've been testing this extensively and it seems to be working in all configurations. There are quite a few improvements compared to what I have on my site right now. Notes:

  • Only compatible with header layout "flex" so far. I have no idea where to put it with "center" and how to style it there.
  • Some files have search as base name - these are generic and could theoretically work unchanged with any other search provider. Others are called lunr-search - that's the lunr-specific code.
  • I corrected the ellipsis in the locale files, this is an unrelated change - meant to use it originally but ended up not doing it after all.
  • Locales for Brazilian Portuguese and Chinese would need to be updated, also the Chinese config example. That's not something I can do.
  • The rudimentary pluralization algorithm in lunr-search.js should do for the locales that currently exist. Cyrillic languages for example have more complicated pluralization rules, but I guess that we can think about that once we actually have the issue.
  • Summary length is hardcoded at 70. That's the default value for summaryLength in Hugo configuration, reading out the actual value is sadly not possible.

@palant palant changed the title feat: Added search capability via lunr feat: Added search capability via lunr (fixes #163) May 19, 2020
@palant palant mentioned this pull request May 19, 2020
Closed
@palant palant requested a review from reuixiy May 19, 2020 11:05
@reuixiy
Copy link
Owner

reuixiy commented May 19, 2020

Awesome! I will review them asap!

@reuixiy
Copy link
Owner

reuixiy commented May 20, 2020 •
edited
Loading

Okay, I just tested it and it works very well, except for the followings:

  1. Cannot return to the "original" page unless refreshed
  2. It seems that currently only supports English

For the 1st, I found the following examples that look good:

  1. https://www.forsure.dev/-/2019/09/03/add-search-functionality-to-your-hugo-static-site/
  2. https://www.w3schools.com/howto/howto_css_fullscreen_search.asp
  3. https://theme-next.js.org/

@palant
Copy link
Collaborator Author

palant commented May 20, 2020

Cannot return to the "original" page unless refreshed

That should be easy to fix - I'll add the previous page state to the history.

It seems that currently only supports English

What do you mean? If I set defaultContentLanguage configuration option to de I get German text. Is multilingual mode the issue? How does i18n function work there?

@palant
Copy link
Collaborator Author

palant commented May 20, 2020

I tried out a multilanguage setup - multiple versions of the JS files are being generated there. So the German pages link to the German-language script which has German-language texts. What am I missing?

@reuixiy
Copy link
Owner

reuixiy commented May 20, 2020

Multilingual mode works well. I mean the search functionality, seems not working for Chinese.

@reuixiy
Copy link
Owner

reuixiy commented May 20, 2020

Only compatible with header layout "flex" so far. I have no idea where to put it with "center" and how to style it there.

Maybe we can solve this by treating search link in menu as a toggle, instead of an input bar. So it will be a toggle, then click, then pop up a full screen overlay on the page, then input...

But this requires lots of code refactoring.

@palant
Copy link
Collaborator Author

palant commented May 20, 2020

I mean the search functionality, seems not working for Chinese.

Oh, so lunr itself doesn't know what to do with Chinese? 😞

@palant
Copy link
Collaborator Author

palant commented May 20, 2020

Found https://github.com/MihaiValentin/lunr-languages - that's the solution for additional lunr languages apparently. Will need to see how this can be incorporated. No Chinese however...

@reuixiy
Copy link
Owner

reuixiy commented May 20, 2020

Oh, so lunr itself doesn't know what to do with Chinese? 😞

Yeah, it just returns empty result...

@palant
Copy link
Collaborator Author

palant commented May 20, 2020 •
edited
Loading

There is a pull request for Chinese but for some reason it's just sitting there: MihaiValentin/lunr-languages#53

Edit: Maybe the lack of progress is due to the nodejieba dependency. I don't think that this project has a good way of bundling dependencies right now.

@palant
Copy link
Collaborator Author

palant commented May 20, 2020

I added some code using History API - a new history entry is created when searching, so that the Back button brings you back to the original content. This is better than overlays, as it's the expected way for search to behave. There are some oddities here still, I'll look into this later.

@palant
Copy link
Collaborator Author

palant commented May 21, 2020

Edit: Maybe the lack of progress is due to the nodejieba dependency. I don't think that this project has a good way of bundling dependencies right now.

Yes, that's definitely the issue here. nodejieba is written in C++ and relies on close to 5 MB in databases. I have no idea whether there is really no simpler way of splitting a Chinese sentence into words, but this definitely isn't something that can run off the web. So, sadly, no lunr for Chinese. 😢

@palant
Copy link
Collaborator Author

palant commented May 21, 2020

Fixed the oddities with going back and forth - the missing part was respecting history.state, now you will get the search again even after reloading the page or after coming back from a different site.

What's missing now is support for non-English languages. I guess that I'll have to list the languages supported by lunr-languages right now and warn if an unsupported language is used.

@palant
Copy link
Collaborator Author

palant commented May 21, 2020

Note that URL doesn't change when you search - this is intentional. I could change it to something like "#search=test" and also respect this anchor when loading. But I doubt that allowing external links to the search is a good idea.

@palant
Copy link
Collaborator Author

palant commented May 21, 2020

There we go, non-English languages will work as well now. If the language isn't supported by the lunr-languages package a warning will be printed at build time.

So all done again.

@reuixiy
Copy link
Owner

reuixiy commented May 21, 2020 •
edited
Loading

Okay, so one last issue, I will be redirected to the search page if I search on the homepage and then click on the homepage.

Update: Firefox seems okay, but Chrome doesn't.

@palant
Copy link
Collaborator Author

palant commented May 21, 2020

Ok, changing URL when searching it is. Looks like no more issues now.

@palant palant merged commit fb54c61 into master May 21, 2020
@palant palant deleted the lunr-search branch May 21, 2020 15:40
@palant
Copy link
Collaborator Author

palant commented May 21, 2020

Noticed an issue on an actual tablet. In landscape mode they have enough space to display the drop-down menu. But when you click the search field the virtual keyboard pops up and occupies lots of vertical screen space - search field loses focus and the menu closes.

@reuixiy
Copy link
Owner

reuixiy commented May 22, 2020

Reproduced. Might be a bug caused by the popped keyboard.

@reuixiy reuixiy mentioned this pull request May 22, 2020
Closed
@reuixiy reuixiy mentioned this pull request Jul 20, 2020
Open
ulmefors pushed a commit to ulmefors/hugo-theme-meme that referenced this pull request Nov 22, 2020
@palant @reuixiy @ulmefors
feat: Added search capability via lunr (reuixiy#165)
ad6324e 
Fixes reuixiy#163 

* feat: Added search capability via lunr

* doc: Fixed typo

* chore: Updated resource files of the example site

* chore: Remove unnecessary div wrapping search results

* chore: Reverted change of the config documentation

* chore: Ordered config entries correctly

* fix: Update document title when displaying search results

* fix: Handle a 404 error when downloading search index

* Use menu variable for icon

* Adjust search icon's style

* fix: Make back button work for restoring previous page state

* fix: Respect search state even when coming back from another page or site

* fix: Support non-English languages

* chore: port config.toml to zh-cn

* i18n: add zh, pt-br translations

* Increase `postWidth` and `listWidth` to `39em`

Otherwise search box will warp to a new line.

* Update screenshots

* Update resources

* fix: Update URL when searching

Co-authored-by: reuixiy <reuixiy@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reuixiy reuixiy reuixiy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@palant @reuixiy