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.

Sign up for GitHub

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

Jump to bottom

fix: algolia search icon on homepage #3373

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

fix: algolia search icon on homepage #3373

Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
c364685
fixed algolia search icon on homepage
amanbhoria Nov 7, 2024
12afe08
Merge branch 'master' into fixAlgoSearch
amanbhoria Nov 8, 2024
55528ab
Merge branch 'master' into fixAlgoSearch
amanbhoria Nov 8, 2024
baa94ef
Merge branch 'master' into fixAlgoSearch
amanbhoria Nov 8, 2024
2e19f66
Merge branch 'master' into fixAlgoSearch
amanbhoria Nov 9, 2024
6791234
initial commit
amanbhoria Nov 10, 2024
e9b34d1
Update NavBar.tsx
amanbhoria Nov 10, 2024
2570d9a
Merge branch 'master' into fixAlgoSearch
amanbhoria Nov 10, 2024
4155980
Update NavBar.tsx
amanbhoria Nov 10, 2024
6100fab
Merge branch 'master' into fixAlgoSearch
amanbhoria Nov 11, 2024
1be152f
Merge branch 'master' into fixAlgoSearch
amanbhoria Nov 12, 2024
0362dfb
Merge branch 'master' into fixAlgoSearch
amanbhoria Nov 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions components/AlgoliaSearch.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,9 +310,7 @@ export function SearchButton({ children, indexName = INDEX_NAME, ...props }: ISe
}, [onInput, searchButtonRef]);

useEffect(() => {
if (typeof children === 'function') {
setChildren(children({ actionKey }));
}
setChildren(typeof children === 'function' ? children({ actionKey }) : children);
Copy link

@coderabbitai coderabbitai bot Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add missing dependencies to useEffect.

The effect uses children and actionKey but they're not included in the dependency array. This violates the React hooks exhaustive deps rule and could lead to stale closures.

Apply this fix:

    setChildren(typeof children === 'function' ? children({ actionKey }) : children);
-  }, []);
+  }, [children, actionKey]);

Note: If you need the effect to run only once, consider memoizing the children and actionKey values using useMemo.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
setChildren(typeof children === 'function' ? children({ actionKey }) : children);
setChildren(typeof children === 'function' ? children({ actionKey }) : children);
}, [children, actionKey]);

Copy link
Author

Choose a reason for hiding this comment

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

the component is being rendered on every button click on Search and so as useEffect. so what's the need to add these parameters in the dependency array? It doesn't make any sense to me

Copy link

Choose a reason for hiding this comment

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

@amanbhoria, understood. If the component re-renders on every button click and the useEffect runs accordingly, then adding these dependencies might not be necessary in this case. Thank you for the clarification.

✏️ Learnings added 
Learnt from: amanbhoria
PR: asyncapi/website#3373
File: components/AlgoliaSearch.tsx:313-313
Timestamp: 2024-11-11T21:30:32.237Z
Learning: In the `SearchButton` component within `components/AlgoliaSearch.tsx`, if the component re-renders on every button click and the `useEffect` runs accordingly, adding dependencies to the dependency array might not be necessary.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

}, []);

return (
Expand Down
Loading