-
Notifications
You must be signed in to change notification settings - Fork 143
feat: Add copy to clipboard Heading #1999
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
feat: Add copy to clipboard Heading #1999
Conversation
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 PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
Comment @cursor review or bugbot run to trigger another review on this PR
| aria-label={anchorTitle} | ||
| > | ||
| <LinkIcon size="16" /> | ||
| </a> |
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.
Bug: Anchor Link Renders Incorrectly Without ID
The component renders an anchor link and copy button even when the id prop is undefined or null. This causes the anchor's href attribute to become "#undefined", while the copy-to-clipboard functionality handles a missing id differently. This inconsistency leads to broken navigation and unexpected copied URLs.
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.
Seems reasonable to add nullable check for id
|
Preview for this PR was built for commit |
katzino
left a comment
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.
LGTM 🤝
Have some smaller nits for you to consider
| if (id) { | ||
| brokenLinks.collectAnchor(id); | ||
| } |
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 would call this method directly inside component's body. useEffect might be a proper place
| aria-label={anchorTitle} | ||
| > | ||
| <LinkIcon size="16" /> | ||
| </a> |
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.
Seems reasonable to add nullable check for id
|
|
||
| .headingCopyIcon { | ||
| display: none; | ||
| transform: translateX(0.25rem); |
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 don't think this line does anything
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.
It actually made a space from the left, but I used a different approach
HonzaTuron
left a comment
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.
Is hook necessary?
|
There is some error in the build 👀 |
|
Preview for this PR was built for commit |
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.
It copied: [object Object] 😄
Also, the copied state does not show or just flickers
Screen.Recording.2025-10-13.at.13.51.18.mov
|
Preview for this PR was built for commit |
|
Preview for this PR was built for commit |
webrdaniel
left a comment
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.
Let's go for that ✊
closes: #1993 (comment)
Create a new
Headingcomponent using the Swizzling method.Updated how the "Copy to clipboard" buttons look so it is consistent with the web.
Also had an issue with scrolling to the desired
id, so I had to useBrokenLinks hook