-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
feat: inline copy feedback for code blocks #8458
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
👋 Codeowner Review RequestThe following codeowners have been identified for the changed files: Team reviewers: @nodejs/nodejs-website Please review the changes when you have a chance. Thank you! 🙏 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8458 +/- ##
==========================================
+ Coverage 73.69% 74.28% +0.59%
==========================================
Files 108 106 -2
Lines 9210 9112 -98
Branches 313 307 -6
==========================================
- Hits 6787 6769 -18
+ Misses 2421 2341 -80
Partials 2 2 ☔ View full report in Codecov by Sentry. |
mikeesto
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.
Looks good, thanks! Could you please run prettier on apps/site/components/Common/CodeBox.tsx to fix CI?
ovflowd
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.
I believe you can remove the whole source of NotificationProvider, including tests, stories and remove it from the Layout too. Which would also allow you to remove the "use client" from the layout. (I think?)
|
Hi @cnaples79! Thanks for the PR, let me know if you need help with @ovflowd's request above, i'd be happy to push a commit helping :-) |
Co-Authored-By: cnaples79 <cnaples79@gmail.com>
Signed-off-by: Aviv Keller <me@aviv.sh>
|
Lighthouse Results
|
| </> | ||
| ) : ( | ||
| <> | ||
| <CodeBracketIcon className="size-4" /> |
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.
What is this icon?
Also nit: (Cleared and less code)
const ButtonIcon = copied ? DocumentDuplicateIcon : CodeBracketIcon;
...
buttonContext={
<>
<ButtonIcon className="size-4" />
{t(copied ?
'components.common.codebox.copied' :
'components.common.codebox.copy'
)}
</>
}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.
What is this icon?
What do you mean? This is the icon we used previously, is it not?
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.
Yeah, I realized that afterwards.
| codeLanguage | ||
| ); | ||
|
|
||
| // Adds a Copy Button to the CodeBox if requested as an additional parameter |
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.
🤔
ovflowd
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.
These changes diverge so much from the original PR provided by OP that these should be a new PR. IMO, with how distinct HEAD is now from the previous commit, this completely removes the "co-author" factor from the original PR author.
Sorry! I meant to help OP expand their PR by removing the NotificationProvider + fixup some Code box changes, I definitely did not mean to overtake this PR 😓 |
It's fine to address some changes, but the extra additions completely overtake the PR, it's "fine", I do believe OP abandoned the PR (possibly), but please make a new PR. (and if possible revert your commit in this PR), then we approve yours, merge and close this one. |
Summary
Rationale
Changes
Fixes #8357