feat(circuit-ui): add CopyButton component#3583
Conversation
🦋 Changeset detectedLatest commit: 0f0ad3d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Size Change: +834 B (+0.1%) Total Size: 837 kB 📦 View Changed
ℹ️ View Unchanged
|
3810417 to
54be451
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3583 +/- ##
==========================================
+ Coverage 93.73% 93.76% +0.02%
==========================================
Files 199 200 +1
Lines 4486 4506 +20
Branches 1763 1767 +4
==========================================
+ Hits 4205 4225 +20
Misses 254 254
Partials 27 27
🚀 New features to boost your workflow:
|
54be451 to
33a19e6
Compare
|
Hi Mat 👋
We will add the component to Figma and in the meantime don't hesitate to ask questions directly to the team :) |
33a19e6 to
4cf120e
Compare
4cf120e to
1b04314
Compare
1b04314 to
85ae3e0
Compare
85ae3e0 to
562ad42
Compare
| * | ||
| * @default input | ||
| */ | ||
| copyVariant?: 'input'; |
There was a problem hiding this comment.
note: variant would be consistent with other components but would also mean we can't use variant when rendering the CopyButton and Button.
There was a problem hiding this comment.
At first I thought: "Let's use size!" and then I realized that it would have the same issue 😅
How about style? The TimeInput uses formatStyles with "long", "short", and "narrow" values.
562ad42 to
317025c
Compare
317025c to
deaf7c9
Compare
deaf7c9 to
ae2ee21
Compare
| ).toHaveAttribute('aria-disabled', 'true'); | ||
| }); | ||
|
|
||
| it('should have no accessibility violations', async () => { |
There was a problem hiding this comment.
Please add an accessibility test for each of the variants.
There was a problem hiding this comment.
Added, thank you. Wasn't sure how to structure it in the tests so please take a look if my solution matches your idea.
| } = props; | ||
| const fieldId = customId || generatedId; | ||
| const displayText = text ?? value; | ||
| const buttonLabel = `${copyLabel}: ${props.label}`; |
There was a problem hiding this comment.
What's the intention behind merging these labels together?
There was a problem hiding this comment.
So I wasn't sure how this should behave from the accessibility perspective - we render an input with a value and then inside it a button to copy the value. Will it be clear from the hierarchy and the button being described with e.g. "Copy to clipboard" that the input is what it will copy?
| variant="tertiary" | ||
| disabled={isCopyDisabled} | ||
| onClick={handleCopy} | ||
| aria-label={buttonLabel} |
There was a problem hiding this comment.
The label for an IconButton should be passed as its children. I'm also confused why the aria-label and children props differ?
| type="button" | ||
| disabled={isCopyDisabled} | ||
| onClick={handleCopy} | ||
| icon={CopyPaste} |
There was a problem hiding this comment.
Idea: We could add a tooltip with the value to the "button" and "icon-button" variants. Alternatively, we could render it in a visually hidden element and associate it with the button via the aria-describedby attribute.
What do you think?
There was a problem hiding this comment.
Opted for aria-describedby as the simpler approach for now.
We have a few places in our dashboard where we render a text and allow user's to copy it into a clipboard using a button. Currently, those are implemented inconsistency, sometimes using the `Input` component, sometimes using custom styling of `Body` and `IconButton`. This change introduces a new ClipboardText component that render a copiable text.
Co-authored-by: Connor Bär <connor-baer@users.noreply.github.com>
1d16abc to
d949561
Compare
| type InputCopyButtonProps = CommonCopyButtonProps & | ||
| Omit< | ||
| InputProps, | ||
| 'defaultValue' | 'onCopy' | 'renderSuffix' | 'showValid' | 'type' | 'value' |
There was a problem hiding this comment.
we can also omit readOnly as its true by default
| 'defaultValue' | 'onCopy' | 'renderSuffix' | 'showValid' | 'type' | 'value' | |
| 'defaultValue' | 'onCopy' | 'renderSuffix' | 'showValid' | 'type' | 'value' | 'readOnly' | 'passwordManagerIgnore' |
| }, | ||
| disabled: { control: 'boolean' }, | ||
| hideLabel: { control: 'boolean' }, | ||
| text: { control: 'text' }, |
There was a problem hiding this comment.
text doesn't seem to be a valid prop of the component 🤔
There was a problem hiding this comment.
Yeah, same as below, forgot to update after refactor 🙈 fixed
| }; | ||
|
|
||
| type ButtonCopyButtonProps = CommonCopyButtonProps & | ||
| Omit<ButtonProps, 'children' | 'icon' | 'onClick' | 'type'> & { |
There was a problem hiding this comment.
| Omit<ButtonProps, 'children' | 'icon' | 'onClick' | 'type'> & { | |
| Omit<ButtonProps, 'children' | 'icon' | 'onClick' | 'type' | 'navigationIcon' | 'destructive' > & { |
| </ToastProvider> | ||
| ), | ||
| ], | ||
| argTypes: { |
There was a problem hiding this comment.
Since we have very different props depending on the variant, can we classify them in categories to make them more understandable ? See the example from the autocomplete input.
|
|
||
| MaskedInput.args = { | ||
| ...inputArgs, | ||
| text: 'sk_live_******', |
There was a problem hiding this comment.
| text: 'sk_live_******', | |
| visibleValue: 'sk_live_******', |
There was a problem hiding this comment.
Updated to visibleValue.
|
|
||
| ## Content | ||
|
|
||
| Keep the visible label clear and specific so users understand what will be copied. Use concise copy such as `"Copy token"` or `"Copy secret"`, and customize `copiedBody` when the success message should refer to a specific object. |
There was a problem hiding this comment.
I don't see any references of copiedBody in the code, did you mean successLabel maybe ?
There was a problem hiding this comment.
Yes, forgot to update docs after updating the component. Fixed.
|
|
||
| Keep the visible label clear and specific so users understand what will be copied. Use concise copy such as `"Copy token"` or `"Copy secret"`, and customize `copiedBody` when the success message should refer to a specific object. | ||
|
|
||
| For longer strings, the input variant truncates the visible text while preserving the full copied value. |
There was a problem hiding this comment.
The LongValue story actually shows the entire value, which scenario did you intend to keep ?
There was a problem hiding this comment.
Fixed, it was a copy paste mistake.
| />, | ||
| ); | ||
|
|
||
| fireEvent.click( |
There was a problem hiding this comment.
prefer await userEvent.click over fireEvent.click
| {copyLabel} | ||
| </Button> | ||
| <span id={valueDescriptionId} className={utilClasses.hideVisually}> | ||
| {value} |
There was a problem hiding this comment.
currently value is of type string | (string & readonly string[]) (the latter inherited from ButtonHTMLAttributes. Can we override it to string ?
| }); | ||
|
|
||
| describe('as input', async () => { | ||
| it('should have no accessibility violations', async () => { |
There was a problem hiding this comment.
how about refactoring tests with describe.each to test the behaviours and accessibility on all variants ?
We have a few places in our dashboard where we render a text and allow user's to copy it into a clipboard using a button. Currently, those are implemented inconsistently, sometimes using the
Inputcomponent, sometimes using custom styling ofBodyandIconButton.This change introduces a new ClipboardText component that renders a copiable text.
Screen.Recording.2026-04-26.at.21.57.13.mov