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: Implement useCopyToClipboard hook #883

Closed
wants to merge 1 commit into from

Conversation

pbellon
Copy link

@pbellon pbellon commented Jul 22, 2022

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

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

@pbellon pbellon force-pushed the pr/useCopyToClipboard branch 2 times, most recently from 3d99b72 to 4713313 Compare July 24, 2022 14:00
@codecov
Copy link

codecov bot commented Aug 8, 2022

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%> (ø)

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

New hook inspired by react-use's `useCopyToClipboard`. 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 https://caniuse.com/?search=navigator.clipboard.
Copy link
Contributor

@xobotyi xobotyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. due to component lack of clipboard read - i really dont seee the need in state at all.

    IMO there is only need in returned callback that returns copied text or throw an error in case of any. Current Implementation will lead to unnecessary re-renders, as of my experience - i never dont care what been copied - most of the time it is simple success or error message depending on results.

    So i'd suggest simplifying the hook, but threfore component would be redundant, as it han be replaced with single function.

    Instead of that i can suggest reimplementing this hook as more complex hook useClipboard that will give access to clipboard reads and reaction to clipboard events occured on docment, events list can be configurable for hook to provide maximal granularity of configuration.

  2. clipboard reset and clipboard set can be merged into single function - do reset when undefined received.

  3. copy function can receive any value, as there are loads of stringable objects that are not strings, just perform String(val) before passing to clipboard.

  4. you did not provide any jsdoc for hook, as it is required by contribution guide.

@ArttuOll
Copy link
Contributor

ArttuOll commented Oct 6, 2022

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

@pbellon
Copy link
Author

pbellon commented Oct 6, 2022

@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).

@xobotyi
Copy link
Contributor

xobotyi commented Oct 9, 2022

@ArttuOll you can take and fix my remarks

@ArttuOll ArttuOll added the 🎂 new hook New hook added label Nov 12, 2022
@xobotyi
Copy link
Contributor

xobotyi commented Jan 2, 2023

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

@ArttuOll
Copy link
Contributor

ArttuOll commented Jan 7, 2023

@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.

@xobotyi
Copy link
Contributor

xobotyi commented Jan 8, 2023

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

@ArttuOll
Copy link
Contributor

ArttuOll commented Jan 8, 2023

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?

@xobotyi
Copy link
Contributor

xobotyi commented Jan 8, 2023

and ways to assign clipboard events handlers, yes.

@ArttuOll
Copy link
Contributor

ArttuOll commented Jan 9, 2023

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
Copy link
Contributor

ArttuOll commented Jan 14, 2023

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.

@xobotyi
Copy link
Contributor

xobotyi commented Jan 18, 2023

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>,
]

@ArttuOll
Copy link
Contributor

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?

@xobotyi
Copy link
Contributor

xobotyi commented May 14, 2023

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.

@ArttuOll
Copy link
Contributor

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.

@xobotyi xobotyi closed this May 17, 2023
@JoeDuncko
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎂 new hook New hook added
Development

Successfully merging this pull request may close these issues.

4 participants