-
-
Notifications
You must be signed in to change notification settings - Fork 8.5k
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
feat(theme-algolia): add option.replaceSearchResultPathname to process/replaceAll search result urls #8428
feat(theme-algolia): add option.replaceSearchResultPathname to process/replaceAll search result urls #8428
Conversation
✅ [V2]
To edit notification comments on pull requests, go to your Netlify site settings. |
⚡️ Lighthouse report for the deploy preview of this PR
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Agree this feature could be useful, and I would probably use it myself
Not 100% fan of the API as it's not very flexible and the naming doesn't feel right
If we can make it better how let's do this. Otherwise I won't block this PR too much because it's a useful but quite niche feature and we can do breaking changes later if needed.
packages/docusaurus-theme-search-algolia/src/theme/SearchBar/index.tsx
Outdated
Show resolved
Hide resolved
packages/docusaurus-theme-search-algolia/src/validateThemeConfig.ts
Outdated
Show resolved
Hide resolved
Thank you for your feedback, I agree with all your suggestions and have implemented them in the latest commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Can you add this to our localhost + deploypreview? That would make sure that we use our own new feature and we could be sure it works end 2 end with the preview.
Example search result:
- https://deploy-preview-8428--docusaurus-2.netlify.app/docs/next/docusaurus-core/
=> - https://deploy-preview-8428--docusaurus-2.netlify.app/docs/docusaurus-core/
I guess this should work: {from: "/docs/next", to: "/docs"}
That makes sense 👍🏻 I've updated the config to use this in dev- and preview mode. And made it possible to provide a regexp or a string in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not super fan of the getRegexpOrString.
I'd prefer either a better solution or to have it removed and just support regexp strings.
I'm a bit afraid this might lead to confusing behaviors when using special chars in from
including [({&+... I'd rather let the user decide explicitly if they want to use a RexExp or a simple string replacement
* @param possibleRegexp string that is possibly a regex | ||
* @returns a Regex if possible, otherwise the string | ||
*/ | ||
export function getRegexpOrString(possibleRegexp: string): RegExp | string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 I don't really like this implementation and would prefer to just support regexp. Adding unit tests would likely reveal the problematic cases.
My idea was more:
- config accepts both string + RegExp
- config validation normalizes this as a serializable RegExp string
- client-side always use
new RexExp()
The config normalization can be something like:
if (from instanceof string) {
return escapeRegexp(from);
} else if (from instanceof RegExp) {
return from.source;
} else {
throw new Error('unexpected")
}
=> always return a valid Regexp string source
Does it make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense; I wasn't terribly happy with getRegexpOrString
either. I've tried to address your suggestions in the latest commit, hope I got it right this time 😅
I'm going to push some little refactors in the PR before merging so please don't commit 🤪 |
- handling of pathname replacement - handling of external urls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 👍
Looks like it works fine on both the search bar and search page, on localhost + deploy preview
…s/replaceAll search result urls #8428
Implemented solution
replaceSearchResultPathname
added - doc: https://deploy-preview-8428--docusaurus-2.netlify.app/docs/search/#connecting-algoliaPre-flight checklist
Motivation
We have an upcoming documentation site we're building at https://stately.ai/docs/. So we've set our
baseUrl
in the config to/docs/
. However when we do our preview deploys, these are served from root/
.This setup makes it hard to use the search on the preview deploys because it indexes based on the site with a different
baseUrl
.This PR adds the option to replace the item's URL that Algolia returns. So in our case we would like to replace
/docs/
with/
.Test Plan
Test links
Deploy preview: https://deploy-preview-8428--docusaurus-2.netlify.app/
Related issues/PRs