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 support from config only #1662

Merged

Conversation

chalin
Copy link
Collaborator

@chalin chalin commented Aug 11, 2023

  • Closes Algolia search support via config only? #991
  • Adds a configurable default implementation of Algolia DocSearch support
  • Allows multiple search options to be chosen, but warns about it
  • Updates the Search docs
  • Empties layouts/partials/hooks/head-end.html and layouts/partials/hooks/body-end.html from the skeletal Algolia-support code.

Preview: https://deploy-preview-1662--docsydocs.netlify.app/docs/adding-content/search/#algolia-docsearch

[!IMPORTANT]
I've temporarily disabled GSE so that reviewers can preview Algolia DocSearch. I'll remove that change before we merge.

(It has been removed.)

@chalin chalin force-pushed the chalin-im-algolia-out-of-the-box-2023-08-11 branch 2 times, most recently from e43b37e to 1dbcca6 Compare August 11, 2023 15:31
userguide/content/en/docs/adding-content/search.md Outdated Show resolved Hide resolved
{{ define "algolia/head" -}}

{{ if and .Site.Params.search (isset .Site.Params.search "algolia") -}}
<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/@docsearch/css@3"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we better place this in head-css.html?


{{ define "algolia/head" -}}

{{ if and .Site.Params.search (isset .Site.Params.search "algolia") -}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since minimum required version is hugo v0.110 now, what about fetching the docsearch npm resource at build time rather than at runtime? This way, we also have better GDPR support (#1354).
Also, I amended the code so that users now can specify the wanted docsearch version inside thier project's hugo.yaml:

params:
  search:
    algolia:
      version: 3.5.1
Suggested change
{{ if and .Site.Params.search (isset .Site.Params.search "algolia") -}}
{{ if and .Site.Params.search (isset .Site.Params.search "algolia") -}}
{{ $version := "v3.5.1" -}}
{{ with .Site.Params.search.algolia.version -}}
{{ $version = . -}}
{{ end -}}
{{ $cdnurl := printf "https://cdn.jsdelivr.net/npm/@docsearch/css@%s" $version -}}
{{ if eq (resources.GetRemote $cdnurl) nil -}}
{{ errorf "Invalid Algolia version %s, could not retrieve this version from CDN" $version -}}
{{ end -}}
{{ with resources.GetRemote $cdnurl -}}
{{ with .Err -}}
{{ errorf "Error while retrieving resource %q from CDN. Status: %s." $cdnurl .Data.Status -}}
{{ else -}}
{{ $secureCSS := . | resources.Fingerprint "sha512" -}}
<link rel="stylesheet" href="{{ .RelPermalink }}" integrity="{{ $secureCSS.Data.Integrity }}" crossorigin="anonymous" />
{{ end -}}
{{ end -}}
{{ end -}}

{{ partial "hooks/body-end.html" . -}}

{{ define "algolia/scripts" }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as for css: why not retrieving the .js resource at build time?

Suggested change
{{ define "algolia/scripts" }}
{{ define "algolia/scripts" }}
{{ $version := "v3.5.1" -}}
{{ with .Site.Params.search.algolia.version -}}
{{ $version = . -}}
{{ end -}}
{{ $cdnurl := printf "https://cdn.jsdelivr.net/npm/@docsearch/js@%s" $version -}}
{{ if eq (resources.GetRemote $cdnurl) nil -}}
{{ errorf "Invalid Algolia version %s, could not retrieve this version from CDN" $version -}}
{{ end -}}
{{ with resources.GetRemote $cdnurl -}}
{{ with .Err -}}
{{ errorf "Error while retrieving resource %q from CDN. Status: %s." $cdnurl .Data.Status -}}
{{ else -}}
{{ $secureJS := . | resources.Fingerprint "sha512" -}}
<script src="{{ .RelPermalink }}" integrity="{{ $secureJS.Data.Integrity }}"
crossorigin="anonymous" ></script>
{{ end -}}
{{ end -}}

@chalin chalin changed the title Algolia support from config only Algolia support from config only [REMOVE GSE config change before merging] Aug 17, 2023
@chalin
Copy link
Collaborator Author

chalin commented Aug 17, 2023

Thanks for your valuable feedback @deining. I've updated the UG as you suggested.

Great idea to fetch dependencies at build time. It should be used uniformly for all 3rd-party features! But for now, I would rather keep this PR simple and consistent with Docy's current way of supporting 3rd-party features/services. There is a plan to eventually address GDPR requirements, and better (modular) handling of 3rd-party dependencies. I'd rather keep that separate.

As for using head-css.html: it is (currently) used for a different purpose, and this PR isn't setting a precedent by modifying head.html directly -- Prism CSS is handled in head.html too.

So I'd leave the rest of this PR as is for now.

@deining
Copy link
Collaborator

deining commented Aug 17, 2023

Thanks for your valuable feedback @deining.

You are welcome.

I've updated the UG as you suggested.

Great!

Great idea to fetch dependencies at build time. It should be used uniformly for all 3rd-party features! But for now, I would rather keep this PR simple and consistent with Docy's current way of supporting 3rd-party features/services. There is a plan to eventually address GDPR requirements, and better (modular) handling of 3rd-party dependencies. I'd rather keep that separate.

As for using head-css.html: it is (currently) used for a different purpose, and this PR isn't setting a precedent by modifying head.html directly -- Prism CSS is handled in head.html too.

So I'd leave the rest of this PR as is for now.

Agreed. I have more Algolia related improvements in the pipeline, let's get this merged first!

@chalin chalin requested a review from deining August 17, 2023 14:30
@chalin chalin force-pushed the chalin-im-algolia-out-of-the-box-2023-08-11 branch from 82bbc9e to 780e6af Compare August 17, 2023 14:30
@chalin chalin force-pushed the chalin-im-algolia-out-of-the-box-2023-08-11 branch from 780e6af to cfb1d19 Compare August 17, 2023 14:37
@chalin
Copy link
Collaborator Author

chalin commented Aug 17, 2023

Thanks, awaiting your approval @deining.

Copy link
Collaborator

@deining deining left a comment

Choose a reason for hiding this comment

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

LGTM.

@chalin chalin changed the title Algolia support from config only [REMOVE GSE config change before merging] Algolia support from config only Aug 17, 2023
@chalin chalin merged commit c5fd935 into google:main Aug 17, 2023
5 of 6 checks passed
@chalin chalin deleted the chalin-im-algolia-out-of-the-box-2023-08-11 branch August 17, 2023 15:06
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.

Algolia search support via config only?
2 participants