fix: address search bar invisible issue in the navbar. #3452
Conversation
souvikjs01
requested review from
derberg,
akshatnema,
magicmatatjahu,
anshgoyalevil,
Mayaleeeee and
asyncapi-bot-eve
as code owners
WalkthroughThe changes in this pull request focus on the Changes
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-3452--asyncapi-website.netlify.app/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
components/AlgoliaSearch.tsx (1)
313-318: Consider simplifying the children rendering logicThe current implementation uses state to store derived data from props, which causes an extra render. Consider simplifying this by computing the children value directly:
- const [Children, setChildren] = useState<string | React.ReactNode>(''); const searchButtonRef = useRef<HTMLButtonElement>(null); const actionKey = getActionKey(); - useEffect(() => { - if (typeof children === 'function') { - setChildren(children({ actionKey })); - } - else { - setChildren(children); - } - }, [children, actionKey]); + const renderedChildren = typeof children === 'function' + ? children({ actionKey }) + : children; return ( <button type="button" ref={searchButtonRef} onClick={() => { onOpen(indexName); }} {...props} data-testid="Search-Button" > - {Children} + {renderedChildren} </button> );This approach:
- Eliminates unnecessary state management
- Reduces component complexity
- Improves performance by avoiding an extra render
- Makes the code more maintainable
🧰 Tools
🪛 eslint
[error] 315-316: Delete
⏎···(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
components/AlgoliaSearch.tsx(1 hunks)
🧰 Additional context used
📓 Learnings (1)
components/AlgoliaSearch.tsx (1)
Learnt from: amanbhoria
PR: asyncapi/website#3373
File: components/AlgoliaSearch.tsx:313-313
Timestamp: 2024-11-12T06:59:33.852Z
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.
| else { | ||
| setChildren(children); | ||
| } |
There was a problem hiding this comment.
Add missing dependency to useEffect hook
The empty dependency array prevents the effect from re-running when the children prop changes, which could cause the search bar to become invisible when the prop updates. This likely explains the issue mentioned in the PR description.
Apply this fix:
- }, []);
+ }, [children, actionKey]);This ensures that the search bar content updates properly when the children prop changes. The actionKey dependency is also needed since it's used within the effect.
Committable suggestion skipped: line range outside the PR's diff.
souvikjs01
force-pushed
the
search-bar
branch
from
2eaa65f to
4c74465
Compare
|
Thanks @souvikjs01 but the issue is resolved in another PR 🚀 |
previously the search bar was missing:
now:
added proper search bar in the navbar and proper functionality.
Summary by CodeRabbit
New Features
Improvements