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

Add clipboard hooks - first approach #85

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Add clipboard hooks - first approach #85

wants to merge 5 commits into from

Conversation

maciekgrzybek
Copy link

Hey, I've added useClipboard hook. No tests yet as I wanted to see if that's good direction with the code. I had some problems with Permission API, as it's still working draft, and types from TS are not exactly valid, but I've managed to figure it out. Let me know what do you think.

Copy link
Owner

@kripod kripod left a comment

Choose a reason for hiding this comment

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

Nice work, thanks for your valuable contribution! Please see my review and change your code accordingly.

.then((permissionStatus: any) => {
// Will be 'granted', 'denied' or 'prompt':
if (permissionStatus.state === 'granted') {
resolve('Permission granted.');
Copy link
Owner

Choose a reason for hiding this comment

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

I think that the resolved value should be a boolean, returning false on failure.

Copy link
Author

Choose a reason for hiding this comment

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

That makes total sense.

@@ -0,0 +1,72 @@
export default function useClipboard() {
const checkForPermission = (type: PermissionName): Promise<string> => {
Copy link
Owner

Choose a reason for hiding this comment

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

This function may be put outside of the useClipboard function (just above it), in order to avoid re-instantiating it on every render.

Copy link
Owner

Choose a reason for hiding this comment

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

On a second thought, checkForPermission may be moved to 'utils.ts', in order to be reused by useDeviceMotion.

Copy link
Author

Choose a reason for hiding this comment

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

Moved.

@@ -0,0 +1,72 @@
export default function useClipboard() {
const checkForPermission = (type: PermissionName): Promise<string> => {
return new Promise((resolve, reject) => {
Copy link
Owner

Choose a reason for hiding this comment

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

Please prefer using async/await.

Comment on lines 7 to 10
navigator.permissions
.query({
name: type,
})
Copy link
Owner

Choose a reason for hiding this comment

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

This should be put on a single line to improve legibility.

Comment on lines 28 to 66
const cut = (element: HTMLInputElement) => {
checkForPermission('clipboard-write' as PermissionName)
.then(() => {
element.select();
document.execCommand('cut');
element.value = '';
element.blur();
})
.catch((error: string) => console.error(error));
};

const paste = (element: HTMLInputElement) => {
checkForPermission('clipboard-read' as PermissionName)
.then(() => {
element.focus();
document.execCommand('paste');
})
.catch((error: string) => console.error(error));
};

const copy = (text: string) => {
checkForPermission('clipboard-write' as PermissionName)
.then(() => {
if (navigator.clipboard) {
navigator.clipboard.writeText(text);
} else {
const tempInput = document.createElement('input');
document.body.appendChild(tempInput);
tempInput.setAttribute('id', 'temp-input');
(document.getElementById(
'temp-input',
) as HTMLInputElement).value = text;
tempInput.select();
document.execCommand('copy');
document.body.removeChild(tempInput);
}
})
.catch((error: string) => console.error(error));
};
Copy link
Owner

@kripod kripod Oct 17, 2019

Choose a reason for hiding this comment

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

Please prefer using async/await and add an optional errorCallback parameter instead of printing on the console.

Copy link
Author

@maciekgrzybek maciekgrzybek left a comment

Choose a reason for hiding this comment

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

Hey @kripod I've made the amends you've mentioned in the comments. Let me know if it's all good :)

@maciekgrzybek
Copy link
Author

Hey @kripod any chance you had some time to look into it?

@codecov
Copy link

codecov bot commented Oct 24, 2019

Codecov Report

Merging #85 into master will decrease coverage by 9.38%.
The diff coverage is 16%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #85      +/-   ##
==========================================
- Coverage   89.27%   79.88%   -9.39%     
==========================================
  Files          29       31       +2     
  Lines         289      348      +59     
  Branches       32       40       +8     
==========================================
+ Hits          258      278      +20     
- Misses         26       66      +40     
+ Partials        5        4       -1
Impacted Files Coverage Δ
packages/web-api-hooks/src/index.ts 100% <100%> (ø) ⬆️
packages/web-api-hooks/src/useClipboard.ts 16.66% <16.66%> (ø)
packages/web-api-hooks/src/utils.ts 60% <7.69%> (-40%) ⬇️
...kages/web-api-hooks/src/usePreferredColorScheme.ts 100% <0%> (ø) ⬆️
...s/web-api-hooks/src/usePreferredMotionIntensity.ts 100% <0%> (ø)
packages/web-api-hooks/src/useMedia.ts 92.3% <0%> (+10.48%) ⬆️
...ckages/web-api-hooks/src/useNetworkAvailability.ts 92.3% <0%> (+15.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 104b818...49225f3. Read the comment docs.

@kripod
Copy link
Owner

kripod commented Oct 24, 2019

Hey @kripod any chance you had some time to look into it?

I apologize for the radio silence, I've been working on a thesis for university, and most of the code I push or accept towards OSS are focused around that. I'll try my best to review and merge this over the weekend. Thank you for the patience!

@maciekgrzybek
Copy link
Author

Ok no worries, I thought you finished that :) don't worry about it now then. And good luck with your thesis :)

@kripod
Copy link
Owner

kripod commented Oct 25, 2019

Thanks, I appreciate you understand.

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.

2 participants