Skip to content
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

Merged
merged 6 commits into from
Aug 1, 2023

Conversation

daKmoR
Copy link
Contributor

@daKmoR daKmoR commented Jul 30, 2023

What I did

  • Add a new component sl-clipboard
  • It shows a copy icon button
  • Component reads for to get text content from an element with an id OR a value directly
  • On click it changes to a different icon and tooltip text "copied"
  • After 2s it resets back to the original copy button and text "copy"
  • Add it to docs
  • Add basics tests (cannot test copy function itself as accessing clipboard in tests is currently not possible 😅)

clipboard

closes #1471

@vercel
Copy link

vercel bot commented Jul 30, 2023

@daKmoR is attempting to deploy a commit to the Font Awesome Team on Vercel.

A member of the Team first needs to authorize it.

@jpzwarte
Copy link

I just happen to have written a similar component last week. My 2 cents: I renamed it from <x-clipboard> to <x-copy-to-clipboard> to make it clearer what it does.

@daKmoR
Copy link
Contributor Author

daKmoR commented Jul 31, 2023

@jpzwarte might make sense... although I doubt there will be more clipboard related components that will be added to shoelace

@claviska what do you think? regarding name and code... e.g. I don't mind a name change... but only makes sense to do it if the component make sense in general 🤔

@vercel
Copy link

vercel bot commented Jul 31, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
shoelace ✅ Ready (Inspect) Visit Preview Aug 1, 2023 6:23pm

Copy link
Collaborator

@KonnorRogers KonnorRogers left a 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);
Copy link
Collaborator

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'}>
Copy link
Collaborator

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.

@claviska
Copy link
Member

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 exportsparts to expose the icon button, but we'd need to pass through both name and library for the actual icons. Actually we'd need to pass through names for the resting state and the copied state — and we're still limited for presentation. (Perhaps I want my app to have <button type="button">Copy to clipboard</button> as the trigger.)

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:

  • Allow full customization of the appearance
  • Eliminate the need for localizing the copy text in the template
  • Let the user control the disabled state (something we're not addressing in this PR)

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 copied or on-copy) is present, we hide the default slot and show the named slot briefly instead. (I'm open to other ideas for this.)

Some other thoughts:

  • I like that we're supporting for to target other elements via id, but I would consider this a more advanced example that should be further down the page
  • It's not clear if and how additional data types (such as images) can be copied. It looks like the Clipboard API supports this now. I'm not sure if it supports fallback content (e.g. if you copy an image and paste into a text field, can you set the fallback text?)
  • We might need to use aria-live to announce the state change when copying, otherwise I don't think screen readers will announce anything
  • A copy event sl-copy should probably be emitted. The detail can contain the resulting promise so users can determine whether or not the copy was successful. It might be useful to include the payload as well.
  • <sl-clipboard> is a bit generic. Does <sl-copy> sound OK to everyone, or should we go with the more explicit <sl-copy-to-clipboard>?

@daKmoR
Copy link
Contributor Author

daKmoR commented Jul 31, 2023

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 🤔

@daKmoR
Copy link
Contributor Author

daKmoR commented Aug 1, 2023

ok I reworked the internal to make it easier to understand

Copy Status

There is now a copyStatus property with possible values of

  • trigger
  • copied
  • error

... I don't think there is a need for a copying state as that state is usually too short to even display. 🤔

Copying none text values

If we want to start copying images, svgs, ... then we might also need a copying status.

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 🤔
Or we need to copy it into a hidden canvas and get the binary format like that.
Anyways both action will potentially require some time therefore needing a copying status.

Overall I would like to do this in a follow up PR/Release.

Support for inputs/textarea/link & shadow dom

done + added to docs

Docs

Showed also error status for wrong for and give an example of when it might fail.

Accessibilty

Added area-live="polite" and seems to work

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)

Events

I now just emit a sl-copying and sl-copied event... but that could be custom event with the attached promise... but I'm not sure what the usecase would be for it? also should the event and/or promise be cancelable? what should it do? 🤔

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 sl-copy right?

@daKmoR
Copy link
Contributor Author

daKmoR commented Aug 1, 2023

found another issue... we can't offer a "public" copy method as a "copy to clipboard action" need to be always triggered via a user action... (otherwise the browser throws an error) => will make the method private

iirc executing it via an attribute worked... but it's kind of a clunky api to do el.setAttribute('copy') instead of el.copy()... maybe not worth it to offer a "programmatic" api before we are sure we absolutely need one... (there are probably a few hacks we could try 😬)

Update:
If a user adds an Event Listener somewhere else and then use the copy method then it works fine. e.g. will leave the method as public.

example:

$0.addEventListener('click', () => {
    document.querySelector('sl-clipboard').copy();
});

@claviska claviska merged commit 16f3e25 into shoelace-style:next Aug 1, 2023
1 check passed
claviska added a commit that referenced this pull request Aug 2, 2023
@claviska claviska mentioned this pull request Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC: new component "sl-copy-to-clipboard"
4 participants