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

SLING-12052: add Pagefind for site search #133

Merged
merged 3 commits into from Oct 2, 2023
Merged

SLING-12052: add Pagefind for site search #133

merged 3 commits into from Oct 2, 2023

Conversation

bdelacretaz
Copy link
Member

This PR adds a search box at the top of the pages, using Pagefind.

Could someone review it before I merge it?

To test it on your local box use

mvn clean package -Ppagefind,run-site

And open http://localhost:8820/

@bdelacretaz bdelacretaz self-assigned this Sep 28, 2023
@kwin
Copy link
Member

kwin commented Sep 28, 2023

Looks really nice, thanks a lot. Is there some way to influence ordering? When I currently search for "models", Sling Models is only the 8th result...

According to https://pagefind.app/docs/weighting/#default-rankings I would have expected that the h1 Sling Models has a much bigger importance....

@kwin
Copy link
Member

kwin commented Sep 28, 2023

We probably need to slightly tweak the markup to consider some head elements (like title) for the search index: https://pagefind.app/docs/metadata/. Otherwise only the body content counts IIUC.

Also we have lots of h1 per page, which all have the same weight, but in fact the first h1 has the highest importance because that one repeats the page title.

@@ -24,7 +24,7 @@ html(lang:'en'){

if( content ) {
if(content.title) {
h1(class:"title") {
h1(class:"title","data-pagefind-body":true) {
Copy link
Member

@kwin kwin Sep 28, 2023

Choose a reason for hiding this comment

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

Wouldn't this restrict indexing to this title only per page(https://pagefind.app/docs/indexing/#limiting-what-sections-of-a-page-are-indexed)/. What about increasing the ranking with data-pagefind-weight="10" (or "9") for this particular h1 and still index the rest of the body?

Copy link
Member Author

Choose a reason for hiding this comment

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

Wouldn't this restrict indexing to this title only per page...

The docs that you point to say "Multiple data-pagefind-body elements may exist on a page, and their content will be combined" - but this didn't work, it looks like pagefind does not index what it finds to be a title, due to it being the first H1 on the page. I have removed this attribute now.

@bdelacretaz
Copy link
Member Author

It looks like pagefind does not index a title that it considers metadata, that's just used when displaying the list of results.

I have now added a display:none; div with the title at a weight of 7.0 and it looks like titles are indexed now.

However, searching for "sling models" still doesn't return the models page as the first result, I don't know why.

@bdelacretaz
Copy link
Member Author

I think this is good enough to commit as a first version, planning to merge this PR early next week unless someone objects.

@bdelacretaz bdelacretaz merged commit f2544c6 into master Oct 2, 2023
1 check passed
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.

None yet

2 participants