feat(search): implements Orama searchbox #6908
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Sorry @micheleriva this has been idle for too long - it's been a messy few weeks. Are you looking for any particular feedback on the approach? |
github-actions
bot
removed
the
github_actions:pull-request
|
Looks like this is a draft yet, and most of the styles feel broken 🤔 |
Unit Test Coverage Report
Unit Test Report
|
|
Hey @bmuenzenmeyer, @ovflowd I'm sorry, I completely missed your comments. We found some bugs in the searchbox and dedicated more time to stabilize it. We've been testing it for weeks on other websites so we believe it is finally ready. I still have to fix a small couple of things, then I'll open it for review 🙏 |
github-actions
bot
removed
the
github_actions:pull-request
|
FYI @micheleriva build is failing: Did you try a prod build locally? |
rjborba
force-pushed
the
feat/adds-orama-searchbox
branch
from
18f2004
to
b279f6f
Compare
github-actions
bot
removed
the
github_actions:pull-request
|
It feels like the results are less obvious. |
|
Also it'd be great if there was some margin between the results |
This comment was marked as resolved.
This comment was marked as resolved.
github-actions
bot
removed
the
github_actions:pull-request
rjborba
force-pushed
the
feat/adds-orama-searchbox
branch
from
d1b2fcd
to
84d73c8
Compare
rjborba
force-pushed
the
feat/adds-orama-searchbox
branch
from
84d73c8
to
3318846
Compare
|
@ovflowd FYI we also started using next router to navigate. Experience should be smoother now. ✅ |
github-actions
bot
removed
the
github_actions:pull-request
|
@rjborba, this is just an FYI: the AI-generated Markdown links are still broken. This can be done in a follow-up PR, but just a FYI, They don't have the language prefixes. |
|
Also, I wonder how much extra bundle is added just for the search box. Can you give us some insights? We want to ensure that the users' experience is smooth. |
Hi @ovflowd it is actually fixed. But it takes in consideration the BASE_URL constant, which is resolving for something different than the ones on the Markdown answer on Next Preview. We've added a callback to the Markdown URL so we have a opportunity to modify the HREF in runtime, after the LLM generates it. We want to add the locale when URL starts with the base URL (https://nodejs.org) and does not have /docs as initial path. We could enhance it by using something like |
rjborba
force-pushed
the
feat/adds-orama-searchbox
branch
from
3318846
to
a105511
Compare
| onSearchResultClick={event => { | ||
| event.preventDefault(); | ||
|
|
||
| const currentPageHost = `${window.location.protocol}//${window.location.host}`; |
There was a problem hiding this comment.
Can we not use window. APIs here at all, I'm pretty sure @/navigation's useRouter and usePathname have everything you need. Using global environment-based APIs hinders the ability of us to optimise this in the future.
| const currentPageHost = `${window.location.protocol}//${window.location.host}`; | ||
|
|
||
| router.push(`${currentPageHost}/${event.detail.result.path}`, { | ||
| locale: undefined, |
There was a problem hiding this comment.
Explain why this is set to undefined within inline docs
| const { path } = event.detail.source; | ||
|
|
||
| if (!path.startsWith('docs/')) { | ||
| window.open(`${currentPageHost}/${locale}/${path}`, '_blank'); |
There was a problem hiding this comment.
Instead of an if else, make a ternary operator for one single window.open call
| searchParams={DEFAULT_ORAMA_QUERY_PARAMS} | ||
| suggestions={DEFAULT_ORAMA_SUGGESTIONS} | ||
| chatMarkdownLinkHref={({ href }) => { | ||
| if (!href) return href; |
There was a problem hiding this comment.
Let's ensure we always use brackets on if statements.
| const docsBaseUrl = `${currentPageHost}/docs`; | ||
| const lowerCaseHref = href.toLowerCase(); | ||
|
|
||
| // Keep original link if URl should not have locale |
There was a problem hiding this comment.
Can you add the reasoning? This whole URL logic seems unnecessary, you can simply check if the href's pathname startsWith docs. You can accomplish that by
const { pathname } = new URL(href);
There was a problem hiding this comment.
I'm adding a few comments here regarding FE best practices (cc @micheleriva) as I believe this is good learning for your Engineer.
I appreciate tenfold the work done here 🙇 -- I hope my nitpicks here can add value 🙏
|
|
||
| const currentPageHost = `${window.location.protocol}//${window.location.host}`; | ||
| const docsBaseUrl = `${currentPageHost}/docs`; | ||
| const lowerCaseHref = href.toLowerCase(); |
There was a problem hiding this comment.
FYI you don't want to add the lowerCase result on the returning href, you want to keep as much as possible from the original URL, only with the modifications for the "/docs" piece :)
github-actions
bot
removed
the
github_actions:pull-request
|
OOC - Is there any way we can style the search button on the Navigation bar? It feels a bit fatty and different from the OG design 👀 |
Description
As discussed with @ovflowd, this PR implements the official Orama Searchbox.
Validation
Related Issues
Check List
npm run formatto ensure the code follows the style guide.npm run testto check if all tests are passing.npx turbo buildto check if the website builds without errors.