feat: Added generic and SearchBar and Orama powered SearchBar. #5419
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@ShogunPanda is attempting to deploy a commit to the OpenJS Foundation Team on Vercel. A member of the Team first needs to authorize it. |
|
Is this a client-side index that is generated at build time? I am not familiar with Orama or Next.js but I believe @ovflowd suggested multiple alternatives and wanted to avoid client-side processing of search indices. |
|
I also haven't checked this PR yet. But I asked the Orama team for something that allowed us leverage both world. A client-side only on first build but if Server-side is available to leverage that 🤔 Will check if this PR is implementing this 👀 |
|
This PR is opened to both possibilities. |
There was a problem hiding this comment.
I think all the application logic, like performing search and etc should be done in a Hook and utils files.
|
Hey! |
|
No, @AugustinMauroy he doesn't need to. He (or anybody else) can do that on a follow-up PR. As this is out of scope of this PR. |
|
FYI I still need to review the remaining code once I get some time 👀 |
|
No worries. Let me know if I have to do anything elsee |
|
FYI gonna finally review the remaining of the PR this week. |
|
Relates to #5204 |
|
|
||
| setQuery(e.target.value); | ||
| }, | ||
| [setQuery, setResults] |
There was a problem hiding this comment.
| [setQuery, setResults] | |
| [] |
We don't need to add these as any setter resulting from useState will never change.
There was a problem hiding this comment.
Here and below: while I agree on that, I tend to pass all values (including immutable one) to useCallback/useMemo in order to have a quick view of what is used in the function. I don't think it affect performance or are there any cons in doing this I'm not aware of?
| el.querySelector('a')?.focus(); | ||
| el.scrollIntoView({ behavior: 'smooth', block: 'nearest' }); | ||
| }, | ||
| [listRef] |
There was a problem hiding this comment.
Refs never change and don't need to be in deps array.
| const [isExpanded, setExpanded] = useState(false); | ||
| const [query, setQuery] = useState(''); | ||
| const [results, setResults] = useState<SearchResult[]>([]); | ||
| const isEmpty = !results || results.length === 0; |
There was a problem hiding this comment.
I feel like this Component is doing to many things. The interactions between a lot of the state objects is making it complicated. Can we try to use maybe useReducer to see if we can clean things up here?
There was a problem hiding this comment.
I'm -1 on this.
query and results are basically changed at the same time.
expanded is triggered independently.
The only other state is isLoading which is only changed once.
I feel adding adding a reducer would be slightly overengineered.
|
And here I go reviewing this, sorry for the super significant delay, @ShogunPanda! |
There was a problem hiding this comment.
Overall fantastic work done here, @ShogunPanda.
My review has some minor comments and significant concerns, so I'm sadly requesting changes. Mostly are tied to the SearchBar component.
Thank you so much for your effort here! I hope we're able to address these review concerns so we can move forward 💪
| justify-content: end; | ||
| margin: 5px; | ||
|
|
||
| .oramaSearchBarFooterLabel { |
There was a problem hiding this comment.
You can use &_ statements here, so you don't need to add the whole prefix again. (Actually in all the child Sass styles here)
Eg.:
.oramaSearchBarFooter {
&_label {
}
}
| import type { OramaSearchBarProps } from './search'; | ||
| import type { SearchFunction } from '../../../types'; | ||
|
|
||
| const Footer: FC = () => { |
There was a problem hiding this comment.
Can this become a separate component OramaSearchBarFooter.tsx? Just because our approach here is to have one top-level Component per file.
There was a problem hiding this comment.
I guess so, it's pretty self contained. Will move it soon.
| rel="noopener noreferrer" | ||
| className={styles.oramaSearchBarFooterLink} | ||
| > | ||
| {isDark ? ( |
There was a problem hiding this comment.
Maybe we can simplify this to:
const OramaLogo = theme === 'dark' ? OramaLogoDark : OramaLogoLight;
...
<OramaLogo className={styles.oramaSearchBarFooterLogo} />| ); | ||
| }, [schema, index, documentMapper, setSearchFunction]); | ||
|
|
||
| return <SearchBar setup={setup} search={searchFunction} footer={Footer} />; |
There was a problem hiding this comment.
I assume that the SearchBar awaits for the setup function?
|
|
||
| export type DocumentMapper = (doc: Document) => SearchResult; | ||
|
|
||
| export type OramaSearchBarProps = { |
There was a problem hiding this comment.
I believe types should be solely defined on a types.ts file just to separate type definitions from actual search logic
There was a problem hiding this comment.
Not an issue. Will move them.
| database: Orama, | ||
| mapper: DocumentMapper, | ||
| term: string | ||
| ): Promise<SearchResult[]> => { |
There was a problem hiding this comment.
I feel that this could be transformed into a promise. Could we do that?
There was a problem hiding this comment.
Ehm... it is already a promise. What do you mean exactly?
There was a problem hiding this comment.
You’re mixing async with Promises. I’m referring to use a pure Promise approach for this function
There was a problem hiding this comment.
Ehm... Promise it's just the return type. Are looking for something like this?
export const performSearch = (
database: Orama,
mapper: DocumentMapper,
term: string
): Promise<SearchResult[]> => {
return search(database, { term }).then(results => results.hits.map(h => mapper(h.document)));
};
There was a problem hiding this comment.
Yup, that’s what I meant. I also guess we don’t need a return statement as this is an one liner.
Yeah, I meant native promises rather than async/await.
There was a problem hiding this comment.
I'm sadly negatively impressed and shocked with the pure size of this Component.
It is so freaking complex. We should discard this component and do something from scratch.
We don't need to keyboard listeners, handlers and all these things now. We can iterate later.
I know this Component is part of the migration, but since it is so complex, it makes it harder to know what's being migrated and what is not.
If I might suggest, let's discard this component and its styles and feel free to make a new one that allows you to type and render the results.
It would be nice if styles, at least of the search container, could be similar, but the content itself, how it collapses/expands, and how it renders the results up to you.
I'm afraid I have to disagree with a Component with many callbacks, effects, conditions and more. Even the useEffect's super complicated.
For example, the rendering of the list (https://github.com/nodejs/nodejs.org/pull/5419/files#diff-cc0fb6b25cb6dad14226e6daa7b1819a859f6c9e5aa102d9557c52b2db485f67R292) is highly complex; we have two effects (https://github.com/nodejs/nodejs.org/pull/5419/files#diff-cc0fb6b25cb6dad14226e6daa7b1819a859f6c9e5aa102d9557c52b2db485f67R157, https://github.com/nodejs/nodejs.org/pull/5419/files#diff-cc0fb6b25cb6dad14226e6daa7b1819a859f6c9e5aa102d9557c52b2db485f67R173) that are also super complex.
I want also to avoid mixing async/await logic on effects, etc., as much as possible. Otherwise, we're creating way too many side-effects.
TL;DR I will find it hard to approve this PR with this Component as it's way too complex. It will on-the-go scare anyone to modify/contribute to this component and it is massively hard to read and understand what it is doing 😅
There was a problem hiding this comment.
I would also mention that the SearchBar component should probably be broken down in 3 components:
- Search Container
- This is the actual wrapper where you type your query
- Search Results
- The container that renders conditionally when there are results
- Search Result / Search Entry
- The individual element for each result can have different styles
I would also say that SearchResults would have the reference of what is the current highlighted result and handle the keystrokes from going up and down.
And Search Container would handle the keystrokes for expanding/collapsing the search container.
- on that, I would also say we should make our key-stroke hook very generic once we implement it. But again, all keystroke logic can be done on a follow-up PR.
There was a problem hiding this comment.
Ok, I'll try to split components as you suggested.
Will follow up on this I hope within this week.
There was a problem hiding this comment.
Yeah I think by doing that, and simplifying the logic and removing everything that is not necessary we make this whole solution way easier.
There was a problem hiding this comment.
But let’s also remove unnecessary callbacks, effects and try to simplify them as much as possible. I haven’t gone through the whole component but I guess there’s room for improvements there.
There was a problem hiding this comment.
Let's remove this hook from now. It is complex, and the code quality is arguable.
We can think about accessibility on a second pass for this PR. Let's get the basics done, and then we think about invoking the search bar with keystrokes or moving through entries with keystrokes.
| @@ -0,0 +1,12 @@ | |||
| export type SearchResult = { | |||
There was a problem hiding this comment.
Usually, on the types folder we use interfaces whenever possible. So this one should be an interface.
|
Hey @ShogunPanda have you had some time to follow-up on this? Do you need any assistance! Thanks :3 |
|
Nope, I don't need any help at the moment. |
|
I mistakenly deleted my fork. Will send a new PR (which fixes all the suggestions outlined here) soon. Sorry for the mess. |
Description
This PR introduces a Search Bar component and it also introduces a Orama based implementation of the same component.
Check List
npx turbo lintto ensure the code follows the style guide. And runnpx turbo lint:fixto fix the style errors if necessary.npx turbo formatto ensure the code follows the style guide.npx turbo testto check if all tests are passing, and/ornpx turbo test:snapshotto update snapshots if I created and/or updated React Components.