-
-
Notifications
You must be signed in to change notification settings - Fork 817
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(clipboard): add new component sl-clipboard #1473
Conversation
@daKmoR is attempting to deploy a commit to the Font Awesome Team on Vercel. A member of the Team first needs to authorize it. |
I just happen to have written a similar component last week. My 2 cents: I renamed it from |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Overall this looks solid! I'd love to be able to support more than just "value" and be able to use <textarea>, links, etc like the GitHub copy element, but this is a solid start!
Biggest feedback right now is we should probably have an error state in addition to a success state, I know it's rare, but it does happen.
} | ||
} | ||
if (this.value) { | ||
navigator.clipboard.writeText(this.value); |
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 know it doesn't happen often, but I think this should probably either be an async function with a try / catch
,or use .then()
or .catch()
in case copying to clipboard fails.
'clipboard--copy': this.copy | ||
})} | ||
> | ||
<sl-tooltip content=${this.copy ? 'Copied' : '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.
Not a big deal for now, but we should make sure to localize this.
Thanks for the PR! I love the idea of this component, but I'd like to make it more reusable and a bit less opinionated. To review, the API currently looks like this: <p>Copy to clipboard</p>
<sl-clipboard value="shoelace rocks"></sl-clipboard> And the template looks like this: render() {
return html`
<div
part="base"
class=${classMap({
clipboard: true,
'clipboard--copy': this.copy
})}
>
<sl-tooltip content=${this.copy ? 'Copied' : 'Copy'}>
<sl-icon-button
@click=${this.handleClick}
name=${this.copy ? 'file-earmark-check' : 'files'}
label=${this.copy ? 'Copied' : 'Copy'}
></sl-icon-button>
</sl-tooltip>
</div>
`;
} This is pretty rigid. Neither the icon button nor the icon itself can be customized. We can use What if we allowed the user to slot in an arbitrary button? 🤔 <!-- Native button -->
<sl-clipboard value="shoelace rocks">
<button type="button">Copy to clipboard</button>
</sl-clipboard>
<!-- Shoelace icon button -->
<sl-clipboard value="shoelace rocks">
<sl-icon-button name="files" label="Copy to clipboard"></sl-icon-button>
</sl-clipboard>
<!-- Shoelace button -->
<sl-clipboard value="shoelace rocks">
<sl-button>Copy</sl-button>
</sl-clipboard> This would:
The caveat here is that you won't get any feedback when copying. A possible solution for that is, when the button is clicked, if a named slot (maybe Some other thoughts:
|
ok, used a slot approach with 3 slots and states... e.g. copy, copied, copy-error e.g. you can do this now <sl-clipboard value="shoelace rocks">
<button type="button">Copy to clipboard</button>
<button slot="copied">copied</button>
</sl-clipboard>
<br>
<sl-clipboard value="shoelace rocks">
<sl-button>Copy</sl-button>
<sl-button slot="copied">Copied</sl-button>
</sl-clipboard> if you only write <sl-clipboard value="shoelace rocks"></sl-clipboard> the the behaviour is just as before 👍 (imho a good default is important... but true it should be changeable to whatever you need) it's also in the updated docs 👍 not sure how to demo the error state though 🤔 |
ok I reworked the internal to make it easier to understand Copy StatusThere is now a
... I don't think there is a need for a Copying none text valuesIf we want to start copying images, svgs, ... then we might also need a For images we would either need to refetch the image like https://web.dev/patterns/clipboard/copy-images/ which might be ok as it should be cached 🤔 Overall I would like to do this in a follow up PR/Release. Support for inputs/textarea/link & shadow domdone + added to docs DocsShowed also error status for wrong AccessibiltyAdded Tested in voiceover with arc/chrome & safari Note: it initially announces it as "copy, copy" I guess because of the tooltip that pop up... imho okisch (I tried but could not fix it) EventsI now just emit a potential idea class SlCopyEvent extends Event {
copyPromise: Promise<void>;
constructor(copyPromise: Promise<void>) {
super('sl-copy', { bubbles: true, composed: true, cancelable: true });
this.copyPromise = copyPromise;
}
} maybe we can leave the event details out for now and add them in a follow up PR/Release? so I guess the only thing missing is the rename to |
found another issue... we can't offer a "public" iirc executing it via an attribute worked... but it's kind of a clunky api to do Update: example: $0.addEventListener('click', () => {
document.querySelector('sl-clipboard').copy();
}); |
What I did
sl-clipboard
for
to get text content from an element with an id OR a value directlycloses #1471