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(web): use lib/utils/copyToClipboard for share link #7603

Conversation

samholton
Copy link
Contributor

@samholton samholton commented Mar 3, 2024

Using Firefox, I noticed there was no button to copy the link on the create share modal. Upon looking to add it, I noticed there was some logic around if it should be shown or not. I removed module.canCopyImagesToClipboard and used the function in lib which was working in Firefox for me on the system settings page.

before
before firefox
before chrome

after
after chrome
after firefox

@@ -41,27 +41,22 @@

$: shareType = albumId ? SharedLinkType.Album : SharedLinkType.Individual;

onMount(async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hello, why do we remove onMount here?

Copy link
Contributor Author

@samholton samholton Mar 3, 2024

Choose a reason for hiding this comment

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

Because check:svelte complained about it? :) I'm just getting into svelte, so perhaps it needs to remain? No longer needing to make this call within

const module = await import('copy-image-clipboard');

And leaving onMount it complained about no async call

Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

The button won't work when accessed over insecure contexts.

https://developer.mozilla.org/en-US/docs/Web/API/Clipboard

@samholton
Copy link
Contributor Author

The button won't work when accessed over insecure contexts.

https://developer.mozilla.org/en-US/docs/Web/API/Clipboard

I saw that, maybe I need to read some more because its working in my Firefox (both on Debian and Android). The copy lib is used elsewhere, so this would make it more consistent rather than loading the additional module which was hidden from the same Debian Firefox. It seems like there is some logic already built in https://github.com/immich-app/immich/blob/main/web/src/lib/utils.ts#L204

@samholton
Copy link
Contributor Author

Ok, so localhost is considered secure. I setup /etc/hosts record and do get the error message now when clicking the copy button. So I think this is still an improvement to make it more consistent with other areas of the app. Perhaps we could add some logic to the lib function to set visibility and use that.

@samholton
Copy link
Contributor Author

One last note, the old method hides it on my Firefox even over https while the new method shows and the copy functionality works.

@jrasm91
Copy link
Contributor

jrasm91 commented Mar 3, 2024

Yes the old method is for copying an image to the clipboard, which I think uses canvas and other unrelated things.

Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Member

@danieldietzler danieldietzler left a comment

Choose a reason for hiding this comment

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

LGTM

@jrasm91 jrasm91 merged commit 87c3d88 into immich-app:main Mar 4, 2024
24 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants