chipzoller / hugo-clarity

A theme for Hugo based on VMware Clarity
Other
571 stars 263 forks source link

[Bug] Search function doesn't work when the default language is other than 'en'. #371

Open KaitaKoinuma opened 2 years ago

KaitaKoinuma commented 2 years ago

I confirm:

Hugo version

0.101.0

Where is this bug showing up?

In the browser: Hugo builds the site, but something doesn't look right.

Operating system

Ubuntu 20.04

Browser (if applicable)

Chrome 103

Current behavior

For example, if swith DefaultContentLanugage to 'pt' from 'en', search function does not work because Lang prefix is attached to index.json for no-en language and cannot be loaded.

Expected behavior

When load index.json, lang prefix is not attached to it when default language but it is attached to when sub languages.

Steps to reproduce

Set 'DefaultContentLanguage' in config.toml to no-en language and restart hugo server and search function does not work. Please look DevTool and it display error message GET http://localhost:1313/{lang code}/index.json 404 (Not Found).

Relevant log output

GET http://localhost:1313/{lang code}/index.json 404 (Not Found)
SyntaxError: Unexpected token p in JSON at position 4

Related code

https://github.com/chipzoller/hugo-clarity/blob/master/assets/js/search.js line:230-234

window.addEventListener('load', function() {
  const pageLanguage = elem('body').dataset.lang;
  const searchIndexLangSlug = pageLanguage === 'en' ? '': `${pageLanguage}/`;
  const searchIndex = `${searchIndexLangSlug}index.json`;
  fetch(new URL(baseURL + searchIndex).href)

Preferred solution

In the above code, compare pageLanguage with the hard coded 'en', but should be compare with the dynamic default language variable. As one solution, I propose using .Sites.First.Language.Lang instead of hard corded 'en' because I cannot find way to get DefaultContentLanguage from config.toml I don't feel good about that this solution force users to set default language weight in languages.toml to 1, but I could think of no other way. I confimed that this solution works succesfully in my environment.

Other information

I hope this report help other people. Thanks!

stdevel commented 1 year ago

I'm having the same issue. Changing 'en' to my site's default language is my current workaround - after that search is working for both languages (de and en).

Could you please share your line, @KaitaKoinuma? Simply replacing 'en' with .Sites.First.Language.Lang results in a syntax error for me. What did I miss?

McAviti commented 1 year ago

Ahoj @stdevel -- I also did not know the correcty way to refer to the first language, so my workaround was was to replace hardcoded "en" with hardcoded "de", that acchieved the same result.

onweru commented 1 year ago

Hi @McAviti, @stdevel,

If you don't mind, I would appreciate your help testing #401.

KaitaKoinuma commented 1 year ago

Hi @stdevel , sorry for my late reply. I will try to check my lines after my today's work, but @onweru pull request probably help you!

@onweru Thank you for pull request for this issue and replying to stdevel and McAviti.

KaitaKoinuma commented 1 year ago

Hi @stdevel

I suppose that you write

const searchIndexLangSlug = pageLanguage === '{{ .Sites.First.Language.Lang }}' ? '': `${pageLanguage}/`;

and the line work as expected.

.Sites.First.Language.Lang is surrounded by '{{ }}' ? If you did that, please show your lines.

In my source code for verfication, I also used variables.js for using first language as a const variable in search.js.

variable.js

const defaultContentLanguage = '{{ .Sites.First.Language.Lang }}';

search.js

const searchIndexLangSlug = pageLanguage === defaultContentLanguage ? '': `${pageLanguage}/`;
McAviti commented 1 year ago

Hi @McAviti, @stdevel,

If you don't mind, I would appreciate your help testing #401.

@onweru -- did a run with branch <371-fix-search-index-lookup>, and it worked for default lang german -- thank you!

Sciroccogti commented 1 year ago

I also tried #401 with Chinese, it works fine so far.

onweru commented 1 year ago

Thanks for the feedback @Sciroccogti and @McAviti . Will merge.