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: Adding the possibility of copying the link by clicking text and icons #171

Merged
merged 12 commits into from
Jun 23, 2023

Conversation

pierrbt
Copy link
Contributor

@pierrbt pierrbt commented Jun 22, 2023

I added the possibility to copy the link just by clicking the text on the upload dialog, and I also added a small check icon when the copying is done.

Tell me if you think something can be changed. I'll be happy to help for other things on this project, I've already used quite a lot TypeScript with React and also Vanilla NodeJS.

Copy link
Owner

@stonith404 stonith404 left a comment

Choose a reason for hiding this comment

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

Thank you, this is a nice addition!

In the file frontend/src/components/share/modals/showCompletedReverseShareModal.tsx is the exact same component, it would be nice if you can change it there too because else the design is inconsistent.

The best way would be if your create a new component e.g CopyTextField and use it in showCompletedReverseShareModal.tsx and showCreateUploadModal.tsx.

@pierrbt
Copy link
Contributor Author

pierrbt commented Jun 23, 2023

@stonith404, I've made the requested changes, let me know what you think about it

@stonith404
Copy link
Owner

Awesome, thank you! Just one thing; when I want to copy the link again, by pressing on the CopyTextField, it doesn't copy the link again. Is this on purpose or a bug?

@pierrbt
Copy link
Contributor Author

pierrbt commented Jun 23, 2023

when I want to copy the link again, by pressing on the CopyTextField, it doesn't copy the link again. Is this on purpose or a bug?

Yes, you can only copy the text itself once because if the user want to select it anyway, it would notificate 3x or more, so it selects only once on the text, but he can always click on the clipboard if he wants to copy it again.

I found disturbing that it displays multiple notifications if we only wanted to select the link.

I can change it if you want, it's only two lines :

const [textClicked, setTextClicked] = useState(false);

if (!textClicked) {
    copyLink();
    setTextClicked(true);
}

@pierrbt
Copy link
Contributor Author

pierrbt commented Jun 23, 2023

@stonith404

@stonith404
Copy link
Owner

Makes absolutely sense!

@stonith404
Copy link
Owner

Are you able to merge the PR or do I have to do it?

@pierrbt
Copy link
Contributor Author

pierrbt commented Jun 23, 2023

No I don't have write access, even with a review, I can't.

@stonith404 stonith404 merged commit 348852c into stonith404:main Jun 23, 2023
1 check passed
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.

None yet

2 participants