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

#607: Pagefind Search #615

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

brianhawthorne
Copy link

Add proof-of-concept pagefind site search.

Copy link

netlify bot commented May 11, 2024

Deploy Preview for scientific-python-hugo-theme ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 9660b14
🔍 Latest deploy log https://app.netlify.com/sites/scientific-python-hugo-theme/deploys/66a6d57ea4277500080b8114
😎 Deploy Preview https://deploy-preview-615--scientific-python-hugo-theme.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 81
Accessibility: 100
Best Practices: 100
SEO: 83
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@jarrodmillman jarrodmillman added the type: Enhancement New feature or request label May 11, 2024
@brianhawthorne brianhawthorne linked an issue Jun 28, 2024 that may be closed by this pull request
Copy link
Member

@stefanv stefanv left a comment

Choose a reason for hiding this comment

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

Thanks @brianhawthorne! This is looking good already.

Makefile Outdated Show resolved Hide resolved
<link href="/pagefind/pagefind-ui.css" rel="stylesheet">
<script src="/pagefind/pagefind-ui.js"></script>

<style>
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to have this defined outside of the template, in the assets/css dir.

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, I added it under assets/theme-css instead of doc/assets/css since the search feature is provided by the theme and not just the example doc site. That worked fine locally with the serve-dev make target, but it's blowing up in the CI build: https://app.netlify.com/sites/scientific-python-hugo-theme/deploys/66a68856fbc041000831976e

For now I'm reverting to put this CSS back where it was (inline) in the search partial, which worked, and affords the benefit of having (almost) the entire implementation in a single file, for easy delete or modify.

layouts/partials/search.html Outdated Show resolved Hide resolved
.pagefind-ui button {
border: none;
}
.pagefind-ui__result-title.svelte-4xnkmf .pagefind-ui__result-link.svelte-4xnkmf {
Copy link
Member

Choose a reason for hiding this comment

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

Would be better if we didn't have to pull in svelte to make search work, but I can't examine JS yet to see how involved that would be.

Copy link
Author

Choose a reason for hiding this comment

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

It's a nice feature of pagefind that it generates a reasonable search results listing for you out of the box. The downside is if we use that default then we don't have much control over how it is rendered and styled, but I thought it was reasonable to lean on it at least for the proof of concept phase of this ticket.

I wasn't super happy about having to refer to these CSS classes here, especially the little hash-like suffixes, which don't convey an obvious meaning and I could imagine might change between pagefind releases. An alternative would be to apply these two color unset rules (which make the results more legible against dark mode) by coding the selector to match the DOM structure instead of referring to these class names. But that is also brittle since the generated DOM could also change shape between releases.


<dialog class="search-dialog"></dialog>

<script>
Copy link
Member

Choose a reason for hiding this comment

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

Maybe make this standalone js as well and link to it? Not crucial, your call.

Copy link
Author

Choose a reason for hiding this comment

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

I tried but wasn't able to resolve (404) the standalone js file. I put it in assets/js/search.js with the URL "./js/search.js", following the pattern of CSS files under theme-css, but no luck. Also tried adding and putting it under a dedicated assets/theme-js directory, so it would look more identical to the CSS case but that didn't work either.

So I'm inclined to leave the JS here unless we can figure out how to get hugo to serve it in a way that makes sense. But it doesn't seem important enough to spend a lot of time on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Site Search
3 participants