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 search, fix: show search box in the sidebar #1651

Merged
merged 1 commit into from
Aug 11, 2023

Conversation

deining
Copy link
Collaborator

@deining deining commented Aug 9, 2023

When enabling algolia search, only the search box in the navbar appears, the search box in the sidebar doesn't show up. As correctly analysed here, this is due to the fact that currently both search boxes use the same id in their corresponding div element (which violates the principle that ids should be unique(!)).

This PR solves this issue, both search boxes show up with this patch applied. This PR also updates the documentation on algolia search in the user guide: we now have to provide two docsearch functions in the docsearch javascript code.


@deining deining added the search Site search / search engines label Aug 9, 2023
@deining deining added this to the 23Q3 milestone Aug 9, 2023
@deining deining requested a review from LisaFC August 9, 2023 19:56
Copy link
Collaborator

@chalin chalin left a comment

Choose a reason for hiding this comment

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

It completely makes sense that the IDs should be unique!

I'd like to propose that this be solved more simply by using, say, an ordinal (.Ordinal) as a suffix to the ID. Such an approach will confine changes to layouts/partials/search-input.html. It will also make it easier to keep things dry during the initializing calls to docsearch(): we'll be able to iterate from 0 to the number_of_input_searches - 1 in our calls to that function.

WDYT?

(Also, I can't recall if we have an easy way to test this?)

@deining
Copy link
Collaborator Author

deining commented Aug 10, 2023

It completely makes sense that the IDs should be unique!

I'd like to propose that this be solved more simply by using, say, an ordinal (.Ordinal) as a suffix to the ID. Such an approach will confine changes to layouts/partials/search-input.html. It will also make it easier to keep things dry during the initializing calls to docsearch(): we'll be able to iterate from 0 to the number_of_input_searches - 1 in our calls to that function.

WDYT?

While authoring my PR, I had exactly the same idea of using an .Ordinal. Then I decided to another route and ended up with a solution that works both when publishing (via hugo) and previewing (via hugo server).

I tried to alter my PR following your recommendations given above and ended up with this code:

{{ with .Scratch.Get "ordinal" }}
  {{ $.Scratch.Add "ordinal" 1 -}}
  {{ else }}
  {{ $.Scratch.Set "ordinal" 1 -}}
{{ end }}

and for the output of the search box <div>:

<div id="docsearch-{{- sub ($.Scratch.Get "ordinal") 1 -}}"></div>

First I liked this minimal solution. It works perfectly fine when publishing (via command hugo), here I'm getting two div elements with IDs docsearch-0 and docsearch-1.
Soon I realized that is does not work when running hugo server, here, I'm getting two div elements with IDs docsearch-3 and docsearch-4 (for whatever reason). Any ideas why this happens?

How to proceed here?

(Also, I can't recall if we have an easy way to test this?)

Good point. Fortunately, I do have a algolia enabled site at hand, but I don't have a solution how to test this in an easy manner in this repo.

@chalin
Copy link
Collaborator

chalin commented Aug 11, 2023

Thanks for trying all of that out.

To be clear: so both the use of .Ordinal, and the solution you outline above, result in different ordinal values depending on the hugo command used (build or serve)?

That's strange.


Regardless, consider using mod, since we need only two distinct IDs:

<div id="docsearch-{{ mod .Ordinal 2 }}"></div>

Or use ($.Scratch.Get "docsearch-id-num") if .Ordinal doesn't work.

@deining
Copy link
Collaborator Author

deining commented Aug 11, 2023

Thanks for trying all of that out.

You are welcome.

Or use ($.Scratch.Get "docsearch-id-num") if .Ordinal doesn't work.

.Ordinal is availbale in shortcodes only, I didn't manage to get it work in partials.

To be clear: so both the use of .Ordinal, and the solution you outline above, result in different ordinal values depending on the hugo command used (build or serve)?

Yes, the solution outline above results in different ordinal values depending on the hugo command used.

That's strange.

Indeed.

Regardless, consider using mod, since we need only two distinct IDs:

<div id="docsearch-{{ mod .Ordinal 2 }}"></div>

Great idea. I modified my PR accordingly, it's shorter and cleaner now and works like a charm!

Thanks for your input, looking forward seeing this merged soon!

@deining deining requested a review from chalin August 11, 2023 07:17
Copy link
Collaborator

@chalin chalin left a comment

Choose a reason for hiding this comment

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

Much cleaner, as you point out.

A few more comments, see inline.

layouts/partials/search-input.html Outdated Show resolved Hide resolved
layouts/partials/search-input.html Show resolved Hide resolved
userguide/content/en/docs/adding-content/search.md Outdated Show resolved Hide resolved
userguide/content/en/docs/adding-content/search.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@chalin chalin left a comment

Choose a reason for hiding this comment

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

Wonderful, thanks!

@chalin chalin merged commit 5692333 into google:main Aug 11, 2023
5 checks passed
@deining deining deleted the algolia branch November 13, 2023 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
search Site search / search engines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Algolia - Mobile navigation/search
2 participants