web icon indicating copy to clipboard operation
web copied to clipboard

feat: Implement useCopyToClipboard hook

Open pbellon opened this issue 3 years ago • 1 comments

What new hook does?

New hook inspired by react-use's useCopyToClipboard, copies some text to clipboard. It returns an array composed as follow:

  • a copyState that reflects copy process (if successful, erroneous and what data has been copied),
  • copyToClipboard, main callback to copy a string into clipboard
  • resetCopyState, new callback not present in react-use's implementation to easily reset copyState to initial value (all fields undefined)

Countrary to react-use's implementation, this hook does not rely on copy-to-clipboard external dependency. Instead it simply uses navigator.clipboard API which seems to have good support on major browser vendors (see caniuse compatibly table). The noUserInteraction: boolean has been removed from copyState because it seemed specific to copy-to-clipboard usage.

Also added a success: boolean field in copyState to ease usage but this is up to debate since we could do as react-use's implementation and only rely on value not being undefined.

Checklist

  • [x] Have you read contribution guideline?
  • [x] Does the code have comments in hard-to-understand areas?
  • [x] Is there an existing issue for this PR?
    • #33 (useCopyToClipboard)
  • [x] Have the files been linted and formatted?
  • [x] Have the docs been updated?
  • [x] Have the tests been added to cover new hook?
  • [x] Have you run the tests locally to confirm they pass?

pbellon avatar Jul 22 '22 19:07 pbellon

Codecov Report

Merging #883 (7661884) into master (3762f78) will not change coverage. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #883   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           58        59    +1     
  Lines         1003      1029   +26     
  Branches       181       185    +4     
=========================================
+ Hits          1003      1029   +26     
Impacted Files Coverage Δ
src/index.ts 100.00% <100.00%> (ø)
src/useCopyToClipboard/useCopyToClipboard.ts 100.00% <100.00%> (ø)

:mega: Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

codecov[bot] avatar Aug 08 '22 00:08 codecov[bot]

Is this new hook, useClipboard, being worked on by anyone at the moment? If not, I could start implementing it over the weekend.

ArttuOll avatar Oct 06 '22 05:10 ArttuOll

@ArttuOll I'm not working on it so feel free to do so. My goal was to port useCopyToClipboard from react-use, not to create a different one (in terms of API and functionality).

pbellon avatar Oct 06 '22 12:10 pbellon

@ArttuOll you can take and fix my remarks

xobotyi avatar Oct 09 '22 11:10 xobotyi

@ArttuOll next pick this one when you'll have free time🍻

xobotyi avatar Jan 02 '23 21:01 xobotyi

@xobotyi I'm currently sketching the API for useClipboard. What do you think about this?

Return

  • eventHandlers
    • onCopy (event: ClipboardEvent) => void - Event handler for the copy event.
    • onCut (event: ClipboardEvent) => void - Event handler for the cut event.
    • onPaste (event: ClipboardEvent) => void - Event handler for the paste event.
  • setClipboardContent (content: unknown) => void - Populate the clipboard with content.

ArttuOll avatar Jan 07 '23 14:01 ArttuOll

im not sure about returned event handlers - i think usage of ClipboardAPI would be better approach

xobotyi avatar Jan 08 '23 12:01 xobotyi

im not sure about returned event handlers - i think usage of ClipboardAPI would be better approach

You mean something like exposing readText and writeText from navigato.clipboard through the hook?

ArttuOll avatar Jan 08 '23 15:01 ArttuOll

and ways to assign clipboard events handlers, yes.

xobotyi avatar Jan 08 '23 16:01 xobotyi

How about this?

Return

  • eventHandlers
    • onCopy (event: ClipboardEvent) => void - Event handler for the copy event.
    • onCut (event: ClipboardEvent) => void - Event handler for the cut event.
    • onPaste (event: ClipboardEvent) => void - Event handler for the paste event.
  • setClipboardContent (content: unknown) => void - Populate the clipboard with content.
  • readClipboardContent () => Promise<string> - Read clipboard content

and ways to assign clipboard events handlers

I'm confused about this. Does eventHandlers not already cover all the possible clipboard events?

Also, should we go with readText and support only string data or with read with supports non-text content, but isn't supported by Firefox? I would probably go with just readText, because read seems to be too bleeding edge.

ArttuOll avatar Jan 09 '23 17:01 ArttuOll

Rethinking this now, I think I'll start implementing something like this. We can change it when I submit a PR, if we need to.

Return

  • write (newClipText: string, onSuccess: () => void, onFailure?: () => void ) => void - Write text to the clipboard.
  • read (onSuccess: (clipText: string) => void, onFailure?: () => void ) => void - Read text from the clipboard.

ArttuOll avatar Jan 14 '23 12:01 ArttuOll

Yes =) Sorry for long response - been really loadded at work recently.

This is exactly what i've been concerned of - you previous approach offered callbacks that should be passed to desired element which is not seems to be necessary.

The only concern now - it seems no possbility to somehow validate\filter the target when we're dealing with clipboard, at least im not able to see it in offered signature.

Proposal: maybe like this:

[
readText: ()=> Promise<string>,
writeText: (text: string)=> Promise<void>,
]

xobotyi avatar Jan 18 '23 08:01 xobotyi

No problem, we're not in a hurry here.

If the hook would have that return type, what is the benefit of having a hook at all instead of just using those methods from navigator directly? Catching errors if navigator.clipboard is not supported and making sure that the methods are called in the browser? I guess that would justify a hook. Or am I missing something?

ArttuOll avatar Jan 18 '23 18:01 ArttuOll

Given this more thinking lately. Do we even need this hook?

Previously such hook been needed due to poor spreading of clipboad api and usage of third-party libs, which is not the case nowadays. Usually such hook used to copy something to user's clipboard therefore there is no need for state updates since application know the state in advance.

@JoeDuncko @ArttuOll I think i'd vote ditching that hook and add note to migration guide. WEB-API covering whole functionality by itself wouthout need of custom hook.

xobotyi avatar May 14 '23 08:05 xobotyi

Given this more thinking lately. Do we even need this hook?

Previously such hook been needed due to poor spreading of clipboad api and usage of third-party libs, which is not the case nowadays. Usually such hook used to copy something to user's clipboard therefore there is no need for state updates since application know the state in advance.

@JoeDuncko @ArttuOll I think i'd vote ditching that hook and add note to migration guide. WEB-API covering whole functionality by itself wouthout need of custom hook.

This is pretty much what I thought when I started implementing this. The hook would do pretty much nothing of value, so the hook is unnecessary in my opinion also.

ArttuOll avatar May 16 '23 04:05 ArttuOll

Given this more thinking lately. Do we even need this hook? Previously such hook been needed due to poor spreading of clipboad api and usage of third-party libs, which is not the case nowadays. Usually such hook used to copy something to user's clipboard therefore there is no need for state updates since application know the state in advance. @JoeDuncko @ArttuOll I think i'd vote ditching that hook and add note to migration guide. WEB-API covering whole functionality by itself wouthout need of custom hook.

This is pretty much what I thought when I started implementing this. The hook would do pretty much nothing of value, so the hook is unnecessary in my opinion also.

Sorry for the delay. I agree.

JoeDuncko avatar May 21 '23 02:05 JoeDuncko