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

Implements #8263, Adds a re-usable Copy to Clipboard service. #13976

Closed
wants to merge 3 commits into from

Conversation

varunsharma27
Copy link
Contributor

@varunsharma27 varunsharma27 commented Sep 13, 2017

Implements #8263, Adds a re-usable Copy to Clipboard service.
Checked on Firefox, Opera and Chrome.

@elasticmachine
Copy link
Contributor

Can one of the admins verify this patch?

@varunsharma27 varunsharma27 changed the title Implements #8263, Adds a re-usable Clipboard Copying service. Implements #8263, Adds a re-usable Copy to Clipboard service. Sep 13, 2017
@@ -15,6 +15,13 @@
>
View single document
</a>
<a
Copy link
Contributor

Choose a reason for hiding this comment

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

This link isn't keyboard accessible, since it doesn't have an href element. Since this is an action rather than a navigation to a different page, it should be a button (and just styled whatever way you wish): https:/elastic/kibana/blob/master/style_guides/accessibility_guide.md#use-button-and-a-href

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@timroes
Copy link
Contributor

timroes commented Sep 13, 2017

Jenkins, test this

@varunsharma27
Copy link
Contributor Author

@timroes, Change Implemented.

timroes
timroes previously approved these changes Sep 18, 2017
@timroes timroes dismissed their stale review September 21, 2017 17:00

I will be on vacation next week and can't track this, so I rather not block this PR with change for requests, but just comment.

@@ -51,6 +51,13 @@ module.directive('kbnTableRow', function ($compile, $httpParamSerializer, kbnUrl
// when we compile the toggle button in the summary, we use this $scope
let $toggleScope;

$scope.copyToClipboard = (uriescape) => {
let text = window.location.origin + window.location.pathname;
text += '#/doc/' + $scope.indexPattern.id + '/' + $scope.row._index + '/' +
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please use template strings instead of concatenation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@varunsharma27
Copy link
Contributor Author

@timroes Any more issues?

@timroes
Copy link
Contributor

timroes commented Oct 11, 2017

Hey @varundbest,

sorry for the long delay, due to my vacations. Thanks again for your PR. Since we are currently trying to move to a more solid platform that backs Kibana and is not tied into AngularJS anymore (also see #9675), we try not to create that centralized services within the Angular infrastructure anymore.

I talked internally with a few people about this, and we would like to revisit that topic, once the new platform has been merged and use this approach to provide that centralized copy-to-clipboard service. We have also an issue for the copy link to document feature that you added in #8263. We would with the new platform approach (but unfortunately we still need a little bit of time on this) revisit also this feature and your centralized service.

Thus I would close that PR for now, but thanks a lot for the input and contribution 💙

@timroes timroes closed this Oct 11, 2017
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.

3 participants