-
Notifications
You must be signed in to change notification settings - Fork 5
Adds a copy button and truncation to title #1476
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
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
df184c5 to
00cce07
Compare
| import { copyToClipboard } from "@/utils/string"; | ||
|
|
||
| interface CopyTextProps { | ||
| children: string; |
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.
obviously we only want to copy strings and not jsx elements, but something about children: string instead of children: ReactNode feels weird to me. I won't block on this, but I did want to flag it because it seems somewhat like an antipattern
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.
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 did see this also, which is why I'm not blocking. That doesn't make it feel any less weird though 😅
00cce07 to
f6b43b9
Compare
| </Text> | ||
|
|
||
| <Button | ||
| variant={null} |
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 is a red flag to me. A null variant button? Hmmmm. That suggests to me there's a major gap in our coverage of button styles. I think I'd rather see a new variant added than a custom button made in-place via null
camielvs
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.
Not sure about the null button variant but otherwise LGTM
f6b43b9 to
b1298bb
Compare


Description
This may be a little much, but I think its pretty cool (and self contained!)
Added a new
CopyTextcomponent that allows users to copy text to clipboard with visual feedback. Implemented this component in theAppMenuto make the title copyable.Related Issue and Pull requests
Type of Change
Checklist
Screenshots (if applicable)
title.mov (uploaded via Graphite)
before:
after:
Test Instructions
Additional Comments
The
CopyTextcomponent provides the following features: