-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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 copy document link #8384
Add copy document link #8384
Conversation
Can one of the admins verify this patch? |
I need @Bargs to take a look at why the URL is not compiled. I am probably doing something wrong. |
@styfle sorry for the delay here, I'm traveling this week so I might not get a chance to look at this until next week. Just wanted to let you know I haven't forgotten |
@Bargs I understand. I will be traveling next week so I might not see your response 😄 |
<div class="pull-right"> | ||
<a ng-href="#/doc/{{indexPattern.id}}/{{row._index}}/{{row._type}}/?id={{row._id | uriescape}}">View</a> | ||
<span style="display:inline-block; width:5px"><!--spacer--></span> | ||
<a ng-click="copyTextToClipboard('#/doc/{{indexPattern.id}}/{{row._index}}/{{row._type}}/?id={{row._id | uriescape}}')">Copy</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The url isn't getting copied correctly because interpolation bindings {{}}
don't work inside expressions. I would suggest building the URL in javascript and store it in a scope variable to keep the template super simple, so you can just do ng-click="copyTextToClipboard(docUrl)"
and ng-href="{{docUrl}}"
(for the view link).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I updated this to use
$detailsScope.docUrl = `#/doc/${$scope.indexPattern.id}/${$scope.row._index}/${$scope.row._type}/?id=${$scope.row._id}`;
But what is the equivalent of {{row._id | uriescape}}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can access angular filters programmatically http://stackoverflow.com/a/14302334/799841
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that worked! 👍
@@ -77,6 +78,35 @@ module.directive('kbnTableRow', function ($compile) { | |||
|
|||
$detailsScope.row = $scope.row; | |||
|
|||
$detailsScope.copyTextToClipboard = text => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an existing clipboard copy implementation in the codebase already:
this.copyToClipboard = selector => { |
Let's extract that existing logic into a service that both the share UI and this doc link UI can leverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I'll give it a shot. Where should it go and what should it be named? I have a lib I created called copee. Maybe I can make that compatible with angular's import and include it as a dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd put it in its own folder under src/ui/public. Here's a simple example of such a service:
module.service('debounce', ['$timeout', function ($timeout) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I think I can use that to write my own module service. Can you provide an example of debounce
being used in another file? It doesn't appear to be imported like other modules. (remember I'm an angular noob).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! You'll need to do two things:
- Import the new copy module here: https://github.com/Bargs/kibana/blob/9924a58f6915a0c1a9df0e6b0f713482fdf69f32/src/ui/public/autoload/modules.js#L8-L8 This isn't an angular thing, but it's required so that the module gets executed and added to angular's dependency injection context.
- Use the service like a normal angular dependency by adding it to the link/controller function's parameters, for example: https://github.com/Bargs/kibana/blob/81efe3b73419acef3c6fc11062511ce4d7e08d60/src/core_plugins/kibana/public/management/sections/indices/add_data_steps/pipeline_setup/directives/output_preview.js#L39-L39
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I created ui/copee
and inject it into both places: the share
directive and my new detailsScope
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Is the code ready for another review?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but did you see my other comment above about uriescape
? I'm not even sure if that is necessary or how to use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, nope, just answered
@Bargs Thanks for looking at this. Anything else I need to do or are we waiting on someone else to review? |
@styfle sorry, just haven't had a chance to review the most recent changes yet. I'll look at them this week then we'll get one other person to review after that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@styfle left some notes on a few minor things I think we could improve
@@ -0,0 +1,46 @@ | |||
import Notifier from 'ui/notify/notifier'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is based on the copee module, but can we give this service a more descriptive name like clipboard
? It'll be easier for other devs to find in the future if they're looking for this sort of functionality.
ta.cols = 1; | ||
ta.rows = 1; | ||
ta.style.color = 'transparent'; | ||
ta.style.border = 'none'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too bad display: 'none'
stops copying from working. In addition to these precautions let's also give the element absolute positioning and push it way over to the left so that it's off screen and won't interrupt the page flow.
ta.style.position = 'absolute';
ta.style.left = '-999999px';
const a = document.createElement('a'); | ||
a.style.color = 'transparent'; | ||
a.style.border = 'none'; | ||
document.body.appendChild(a); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you even need to append the anchor to the document, the href seems to get generated correctly without it.
document.body.appendChild(a); | ||
a.href = url; | ||
|
||
const ta = document.createElement('textarea'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's give this a more descriptive name like copyElement
location: `Share ${name}`, | ||
}); | ||
|
||
const a = document.createElement('a'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to avoid one letter names, anchorElement
would be better.
copee.urlToClipboard = (url, name) => { | ||
const notify = new Notifier({ | ||
location: `Share ${name}`, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of having a name
string parameter, how about we have a notifier
parameter that takes a notifier instance? Then the calling code can be responsible for creating the notifier with the correct name, and can re-use it outside of this service. In particular, this would allow share.js
to easily do its "URL selected" notification if copying fails which is currently gone in this version of the code.
import uiModules from 'ui/modules'; | ||
// borrowed heavily from https://github.com/styfle/copee | ||
|
||
let module = uiModules.get('kibana'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const
@Bargs Thanks for reviewing. I went ahead and made those changes you requested 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@styfle looking pretty good! I listed a few more minor cleanup items.
One last larger ask: could you add some unit tests for the urlToClipboard
method?
module.service('clipboard', function () { | ||
const clipboard = this; | ||
|
||
clipboard.urlToClipboard = (url, notifier) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notifier isn't getting used anywhere. I think that's ok though, we can remove this param entirely and just allow the calling code to handle notifications.
clipboard.urlToClipboard = (url, notifier) => { | ||
const anchorElement = document.createElement('a'); | ||
anchorElement.style.color = 'transparent'; | ||
anchorElement.style.border = 'none'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two style setters are unnecessary now that we're not injecting the element into the page.
success = false; | ||
} | ||
|
||
document.body.removeChild(copyElement); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using $document everywhere instead of the raw document
object would make testing this service a lot easier.
if (success) { | ||
notify.info('URL copied to clipboard.'); | ||
} else { | ||
notify.info('Failed to copy to clipboard.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use notify.error
here so the failure catches the user's eye.
<div class="pull-right"> | ||
<a ng-href="{{docUrl}}">View</a> | ||
<span style="display:inline-block; width:5px"><!--spacer--></span> | ||
<a ng-click="copyTextToClipboard(docUrl)">Copy</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized just the word "Copy" might be confusing to the user, they might think it's supposed to copy the json or something. How about changing this to "Copy URL"?
@Bargs I made those changes you requested. But I'll have to figure out how the tests and mocks work. How many tests do you want? |
Enough to verify the expected use cases of the
|
Let me know if you need any help figuring out our test setup. To get started you'll want to use |
@Bargs I haven't been able to run the test server, let alone write any tests yet. Here is what I see:
I'm using Git Bash on Windows 10. |
Ah, Windows. @BigFunger or @LeeDr do you know what's required to run the frontend karma based tests on Windows 10? |
I'm on Windows 10. I'll give those tests a try and see if they pass for me. |
The tests ran on my Windows 10 (with a few failures) on Kibana master. I'll looking into the failures, but the tests should run on Windows 10. They launch an IE browser window with a "Debug" button. Click that, opens a new tab where the tests run.
Here's the first 4 failures;JSON input validation ✓ ✖ ✖ ✖ The other 2 were timeout errors. I might try increasing the timeout to see if they would pass in that case;Events ✓ ✓ ✓ ✓ ✓ ✓ ✖ ✓ ✓ ✖ |
46b092f
to
fd0b062
Compare
I was on master, to see if the tests all pass there. I'm planning to try switching the tests locally to use Chrome instead of IE on Windows just as a test to see if the browser is related to the failures I saw (6 in the first run, 8 in the second run). |
@Bargs I just rebased on master and force pushed.
Now I actually see some JSON input validation errors in my browser (it launched MS Edge). |
Some of my Windows/IE test failures are timeouts and I submitted this PR to increase the timeout; Still looking into some other ones (still on master atm) |
jenkins, test it |
I also filed this issue #8973 about 4 tests that fail for me consistently on IE but pass on Chrome. To change your browser look in Gruntfile.js around line 26. |
@styfle unit tests are passing for me locally on OSX and Chrome. Try switching to Chrome as Lee mentioned. I did notice some functional (selenium) test failures that look related to your change though. This one in particular:
You can run these selenium based tests with kibana/test/functional/index.js Line 32 in b053aad
|
@styfle also it looks like you may not have actually pushed after rebasing, or maybe you didn't update your local master branch before rebasing. The last kibana commit in your feature branch is from mid october https://github.com/styfle/kibana/commits/feature/copy-doc-link The error that Jenkins is reporting was fixed by a PR that got merged two days ago. |
@Bargs I did the following.
But now I realize that maybe I need to somehow update master from upstream first? |
Yeah you'll want to update your local master with changes from upstream. I
|
fd0b062
to
9e60ff6
Compare
You might need to |
Hi @styfle, were you still interested in working on this? |
@Bargs I am still interested but I don't have the time. Maybe I can revisit in January. Those pesky unit tests got me down 😞 |
Ok, no worries! Sorry the tests were a bit frustrating. Let me know if I can do anything to help if you decide to pick it back up. I'm going to close this for the time being, but we can reopen if you get time to work on it in the future. |
This adds a link/button next to the document that allows the user to copy the document's URL to clipboard.
This fixes #8263 😄