-
Notifications
You must be signed in to change notification settings - Fork 0
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
Update Sidebar.tsx #1
Conversation
Reviewer's Guide by SourceryThe PR fixes an issue with sidebar message links by updating the anchor href attribute to use message.id() instead of message.name(). This change ensures that the anchor links work correctly regardless of whether message.name() is defined or different from message.id(). No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @parthikjadav - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@@ -38,10 +38,11 @@ export const Sidebar: React.FunctionComponent = () => { | |||
</a> | |||
<ul className="text-sm mt-2"> | |||
{messages.map((message, index) => ( | |||
<li key={`menu-message-list-${message.name() ?? index}`}> | |||
<li key={`menu-message-list-${message.name() ?? index}`}> |
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.
suggestion (bug_risk): Consider using message.id() instead of message.name() in the key prop for consistency
Since message.name() was replaced with message.id() in the href to fix undefined values, the same change should be applied to the key prop to maintain consistency and prevent potential React reconciliation issues.
</a>
<ul className="text-sm mt-2">
{messages.map((message, index) => (
<li key={`menu-message-list-${message.id() ?? index}`}>
fixed this issue asyncapi#1109
Description
Changes proposed in this pull request:
The anchor link for a message in the sidebar does not work as expected when message.name() and message.id() differ.
Expected result
Clicking on a message in the sidebar should set the correct hash in the URL and scroll the message into view.
Actual result
The anchor link only works if message.name() and message.id() are identical.
If message.name() is not provided, the hash becomes #message-undefined.
before
after
Summary by Sourcery
Bug Fixes: