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

feat(search): implements Orama searchbox #6908

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

micheleriva
Copy link
Contributor

Description

As discussed with @ovflowd, this PR implements the official Orama Searchbox.

Validation

Related Issues

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run npm run format to ensure the code follows the style guide.
  • I have run npm run test to check if all tests are passing.
  • I have run npx turbo build to check if the website builds without errors.
  • I've covered new added functionality with unit tests if necessary.

Copy link

vercel bot commented Jul 8, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview Nov 22, 2024 4:14am

@bmuenzenmeyer
Copy link
Collaborator

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?

@ovflowd
Copy link
Member

ovflowd commented Jul 31, 2024

Looks like this is a draft yet, and most of the styles feel broken 🤔

Copy link

github-actions bot commented Jul 31, 2024

Unit Test Coverage Report

Lines Statements Branches Functions
Coverage: 91%
90.14% (576/639) 72.08% (173/240) 93.91% (108/115)

Unit Test Report

Tests Skipped Failures Errors Time
131 0 💤 0 ❌ 0 🔥 5.336s ⏱️

@micheleriva
Copy link
Contributor Author

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 🙏

@micheleriva micheleriva marked this pull request as ready for review August 21, 2024 18:33
@micheleriva micheleriva requested a review from a team as a code owner August 21, 2024 18:33
@ovflowd ovflowd added the github_actions:pull-request Trigger Pull Request Checks label Aug 21, 2024
@github-actions github-actions bot removed the github_actions:pull-request Trigger Pull Request Checks label Aug 21, 2024
@ovflowd
Copy link
Member

ovflowd commented Aug 21, 2024

FYI @micheleriva build is failing:

@nodejs/website:build: Error: 
@nodejs/website:build:   x Expression expected
@nodejs/website:build:      ,-[128:1]

Did you try a prod build locally?

@ovflowd
Copy link
Member

ovflowd commented Aug 29, 2024

image

It feels like the results are less obvious.

@ovflowd
Copy link
Member

ovflowd commented Aug 29, 2024

Also it'd be great if there was some margin between the results

@ovflowd

This comment was marked as resolved.

@rjborba
Copy link

rjborba commented Nov 20, 2024

@ovflowd We've rebased the code and fixed the tab cycle.

About the broken Links, it may be a deeper discussion.
Is there any chance that inside the doc APP, we default the no-language URLs to a default language (english, for example)?

@ovflowd
Copy link
Member

ovflowd commented Nov 20, 2024

Is there any chance that inside the doc APP, we default the no-language URLs to a default language (english, for example)?

I didn't get your question; could you rephrase it?

@RedYetiDev
Copy link
Member

I didn't get your question; could you rephrase it?

I think they mean that https://nodejs.org/some-url will default to https://nodejs.org/en/some-url.

@rjborba
Copy link

rjborba commented Nov 20, 2024

I didn't get your question; could you rephrase it?

I think they mean that https://nodejs.org/some-url will default to https://nodejs.org/en/some-url.

Exactly it. Sorry, I dropped the message pretty late yesterday.

@ovflowd
Copy link
Member

ovflowd commented Nov 20, 2024

I didn't get your question; could you rephrase it?

I think they mean that nodejs.org/some-url will default to nodejs.org/en/some-url.

Exactly it. Sorry, I dropped the message pretty late yesterday.

That's fine by me if these are the links within the MDX AI results

description: ({
path,
pageSectionTitle,
}: {
Copy link
Member

Choose a reason for hiding this comment

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

Can you extract this from an inline type? Thanks

Copy link

Choose a reason for hiding this comment

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

path: string;
pageSectionTitle: string;
}) => {
return getFormattedPath(path, pageSectionTitle);
Copy link
Member

Choose a reason for hiding this comment

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

No return statement and arrow needed just direct => getFormattedPath(path, pageSectionTitle);

Copy link

Choose a reason for hiding this comment

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

Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

I'm happy with this landing as it is. Please just address the final comments and if possible, discard the package-lock.json changes from this PR and rerun npm i and then commit the package-lock

You can do that by doing: git checkout origin/main -- package-lock.json

@ovflowd
Copy link
Member

ovflowd commented Nov 20, 2024

image

Can the hover colour of this be the same of the other buttons on the right? 🙏

@ovflowd
Copy link
Member

ovflowd commented Nov 20, 2024

Also @micheleriva any chance your Search component can allow us to override the anchor <a> element? Any link you click on the Searchbox causes a hard refresh of the page as it is not using Next.js's router :) -- that is bad UX and also makes the experience be less smooth.

I'm fine merging as it is, but would appreciate that change ✨

@ovflowd
Copy link
Member

ovflowd commented Nov 20, 2024

Ah, I thought the fix for the Markdown links was here already? I still see these being broken, is that intended?

image

@micheleriva
Copy link
Contributor Author

@ovflowd I completely understand and agree with you. Will provide a way to avoid full-page refreshes with an update in the coming weeks

@ovflowd
Copy link
Member

ovflowd commented Nov 20, 2024

@ovflowd I completely understand and agree with you. Will provide a way to avoid full-page refreshes with an update in the coming weeks

Oh yeah, no rush, we can follow-up on that <3

@rjborba
Copy link

rjborba commented Nov 21, 2024

Also @micheleriva any chance your Search component can allow us to override the anchor <a> element? Any link you click on the Searchbox causes a hard refresh of the page as it is not using Next.js's router :) -- that is bad UX and also makes the experience be less smooth.

I'm fine merging as it is, but would appreciate that change ✨

Is it fine if you are able to prevent default behavior and use router programatically? Like:

<OramaSearchBox
    onSearchResultClick={(e) => {
        e.preventDefault()
        // Call router to navigate
    }} />

@rjborba
Copy link

rjborba commented Nov 22, 2024

Also @micheleriva any chance your Search component can allow us to override the anchor <a> element? Any link you click on the Searchbox causes a hard refresh of the page as it is not using Next.js's router :) -- that is bad UX and also makes the experience be less smooth.
I'm fine merging as it is, but would appreciate that change ✨

Is it fine if you are able to prevent default behavior and use router programatically? Like:

<OramaSearchBox
    onSearchResultClick={(e) => {
        e.preventDefault()
        // Call router to navigate
    }} />

@ovflowd hi! I think you missed this message

@ovflowd
Copy link
Member

ovflowd commented Nov 23, 2024

Also @micheleriva any chance your Search component can allow us to override the anchor <a> element? Any link you click on the Searchbox causes a hard refresh of the page as it is not using Next.js's router :) -- that is bad UX and also makes the experience be less smooth.
I'm fine merging as it is, but would appreciate that change ✨

Is it fine if you are able to prevent default behavior and use router programatically? Like:

<OramaSearchBox
    onSearchResultClick={(e) => {
        e.preventDefault()
        // Call router to navigate
    }} />

@ovflowd hi! I think you missed this message

Oh, apologies, yeah feel free to implement this :)

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.

Accessibility Issues with Scrolling and Focus in WithSearchBox Modal
9 participants