-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
feat(search): use new Orama components #8175
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Aviv Keller <[email protected]> Signed-off-by: Aileen Villanueva Lecuona <[email protected]>
Co-authored-by: Aviv Keller <[email protected]> Signed-off-by: Aileen Villanueva Lecuona <[email protected]>
Co-authored-by: Aviv Keller <[email protected]> Signed-off-by: Aileen Villanueva Lecuona <[email protected]>
Co-authored-by: Aviv Keller <[email protected]> Signed-off-by: Aileen Villanueva Lecuona <[email protected]>
Co-authored-by: Aviv Keller <[email protected]> Signed-off-by: Aileen Villanueva Lecuona <[email protected]>
Thanks @avivkeller. So I've updated the HTML element targeted as search-results wrapper (I've targeted the wrong one before by mistake). I tried to run the playwright tests locally, and they're passing. |
Can you update https://github.com/nodejs/nodejs.org/blob/main/.github/workflows/sync-orama.yml to use the new API keys? See https://github.com/nodejs/nodejs.org/actions/runs/18316807448/job/52159169199?pr=8175 |
setIsMobileScreen(window.innerWidth < 1024); | ||
}; | ||
checkScreenSize(); | ||
window.addEventListener('resize', checkScreenSize); |
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.
Shouldn't these be passive listeners?
setOpen(!open); | ||
}; | ||
|
||
useEffect(() => { |
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.
@canerakdas is there any Radix component we could use for handling these? Maybe Dialog? https://www.radix-ui.com/primitives/docs/components/dialog
I'd like to avoid wiring manual useEffects and hardcoding these.
{!isMobileScreen && mode === 'chat' && ( | ||
<SlidingChatPanel | ||
open={isChatOpen} | ||
onClose={() => { |
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.
Feels like we should use React Reducers to manage the lifecycle of the SearchBox and Dialog IMO, could we do that?
} | ||
}; | ||
|
||
const handleChatOpened = (): void => { |
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.
All of these feel like lifecycle hooks that could be done either via Hooks or Reducers.
const displaySearch = | ||
!isMobileScreen || (isMobileScreen && mode === 'search'); | ||
|
||
useEffect(() => { |
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.
Could you please use this? https://github.com/nodejs/nodejs.org/blob/main/apps/site/hooks/react-client/useMediaQuery.ts using resize effects is inefficient.
}; | ||
}, []); | ||
|
||
const handleSelectMode = (newMode: 'search' | 'chat') => { |
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.
Lifecycle Hooks 👀
import { Search } from './Search'; | ||
import { SlidingChatPanel } from './SlidingChatPanel'; | ||
|
||
const orama = |
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 feel orama should be instantiated somewhere else and not inside a component. It should probably be provided by a React Provider so that you wrap the search box with <OramaProvider>
and then use const orama = useOrama()
@@ -0,0 +1,11 @@ | |||
export const uppercaseFirst = (word: 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.
Can't this be done with CSS?
> | ||
<DocumentTextIcon /> | ||
<div> | ||
{typeof hit.document?.pageSectionTitle === |
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.
Can you extract this to a proper function? Too much complexity in a single component.
} = useSearch(); | ||
const containerRef = useRef<HTMLDivElement>(null); | ||
|
||
const clearAll = useCallback(() => { |
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.
Lifecycle Hooks (React Reducer)
dispatch({ type: 'SET_RESULTS', payload: { results: [] } }); | ||
}, [dispatch]); | ||
|
||
useEffect(() => { |
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.
Lifecycle Hooks
}; | ||
}, [clearAll]); | ||
|
||
useEffect(() => { |
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'm strongly against using querySelectors within React, it breaks the nature of React.
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.
This component is just too big, please break it down into smaller parts 🙇
|
||
const oramaLogo = `https://website-assets.oramasearch.com/orama-when-${resolvedTheme}.svg`; | ||
|
||
const clearAll = useCallback(() => { |
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.
Lifecycle Hooks
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.
Searching on this branch is broken?

There are a few things here and there I believe need to be addressed. Some components are too big, some have way too many Effects that can become React Reducers for Lifecycle management.
I genuinely appreciate the effort done here ❤️ and 100% value the effort and time Orama team is spending on this PR 🙇
This PR supersedes #7971 and integrates the new Orama components, powered by the new OramaCore backend.
Description
New components - React-based to replace the old WebComponents-based ones. New backend (OramaCore instead of old Orama Cloud), all hosted and maintained on https://app.orama.com. Credentials have been shared privately with the repository maintainers on Slack.
Validation
Tested locally and on remote demo environment.
Related Issues
Check List
pnpm format
to ensure the code follows the style guide.pnpm test
to check if all tests are passing.pnpm build
to check if the website builds without errors.