-
Notifications
You must be signed in to change notification settings - Fork 53
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
Ctrl + C #14
Comments
Hey, sounds nice, do you have an example or some code I can see? |
Alright I added in a demo, and it works when you run it in the browser (Chrome and Firefox) but I cant get the tests to work its like the element cant see the events, I'm sure its a stupid mistake on my part. I can take a look later today but I should probably get back to work for now. I wanted to get something pushed to see if this is the direction you wanted to go before I update the readme and what not. Let me know what you think. |
Thanks for the code, now I understand what you meant :) However, I don't really agree with your approach. Capturing CTRL+C globally in the page is breaking a core convention that CTRL+C copies whatever text is highlighted. This PR will break that convention, making it impossible to reliably copy anything else from the page. I don't know what your specific use case is, but I would probably place the text being copied in an input field or textarea, allowing for normal CTRL+C to copy it without any need for special js handling. |
Yeah I agree that capturing the event globally would be poor practice 👎. But this implementation copies using ctrl+c only when the element is in focus. That is the reason it says to click the box, because it wont copy unless you are focused on that element. You log
The reason why I had to implement this is to press ctrl + c then run a function to copy cells out of ui-grid. I guess you have to leave it up to the Dev to use this directive on a appropriately nested element and to read the docs to see what this is actually doing. Either way we have had it running with no complaints for about 3 months now. |
I pushed an update with a check for selected text. Now if there is selected text on the page the start copy event will not fire. |
Couldn't you still implement this with a normal I still think hijacking CTRL+C to re-implement CTRL+C is a wrong way to go, and could have unintended side effects, so I would prefer to keep it out of this directive. My suggestion would be to implement this as a new directive, which you can then apply on top of angular-clipboard. Then people can opt-in if they would like this functionality. An idea could be to simply trigger the "click" event on the angular-clipboard button, and thus invoke the copy action? |
If I have access to an input box why wouldn't I just select the text in the input box and copy it directly? My use case for this directive is to avoid having the text I want to copy on the screen. When a user presses ctrl c it runs a function and puts the return value in their clipboard. Trello does this when you press ctrl+c with a card open. It copies the url of the card into the clipboard no input box required. The reason why having both of these functions available in a single directive would be good is because the underlying behaviour is the same you let the user decide the initial event that would start the copy. I might not be explaining myself very well and I don't mind this PR being rejected. Especially since this is a decently large change. |
It would be nice if we had the option to show a nice tooltip with text selection like the clipboard.js demo. I'm not sure how you could define this in a generic fashion.. In some of my use cases I'm copying a textbox value in others It's just a button that copys output to the clipboard. |
First off, just wanted to say thanks for this awesome directive. Helped me save a bunch of time at work.
Anyways, I had to implement Ctrl +C for a project and I extended the functionality of this directive.
I have the code with tests written in typescript but I can quickly write it out in javascript and make a pull request. Let me know if that is something you would be interested in adding to the project.
The text was updated successfully, but these errors were encountered: