-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Discover] Show "Copy value" button for any grid cell #149525
Conversation
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
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.
Code review, and pulled & tested locally. Works as expected, and something I've wanted in Discover since I started at Elastic!
Two notes that don't need to be addressed in this PR, but I think are worth considering:
- While this is great to have as is, personally I would find it more useful as an in-cell action for quicker access (maybe too many buttons though), and/or implementing a
Ctrl/Cmd + C
shortcut to copy the focused cell value. - Keyword values are copied exactly as shown in the table, but strings, numeric values, and possibly other types become quoted (e.g.
1,902.22
is copied as"1,902.22"
). This seems to be expected behaviour based onconvertValueToString
, but as a user I found it unexpected and a bit annoying.
cc @kertal and @ryankeairns
I guess in-cell action would mean 3 tiny icons in a row, I think this was possible a while ago, and then it was limited for the "To many buttons" reason. The Ctrl/Cmd + C option sound like a good idea, but should be enabled on EUI level, so this sounds like a good EUI enhancement issue 👍
Ok, I think we're using the output of the field formatters here for this case, and yes, you know when I'd copy paste Kudos @jughosta to implement this so quickly! |
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.
LGTM, just approving since the @elastic/kibana-data-discovery team was still assigned to review although @davismcphee approved, also tested and it works as expected. One note, it's not also possible to copy the whole document, and since there are no filter buttons in this column this can be done directly without expanding:
@davismcphee @kertal Before merging the PR I can look into skipping quotes when copied via the cell popover but keep it when a whole column is copied. |
@davismcphee @kertal Pushed a commit which prevents quotes when copying from a cell popover. This made me thinking maybe we should have 2 options: "Copy" and "Copy value" (original, non-formatted value which can be useful for number, dates, etc). |
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.
Pulled and tested again, and now it's copying as expected! 🎉 Thanks for this, it's great functionality to have.
I guess in-cell action would mean 3 tiny icons in a row, I think this was possible a while ago, and then it was limited for the "To many buttons" reason. The Ctrl/Cmd + C option sound like a good idea, but should be enabled on EUI level, so this sounds like a good EUI enhancement issue 👍
@kertal Good call, I created an EUI issue to request it here: elastic/eui#6561.
This made me thinking maybe we should have 2 options: "Copy" and "Copy value" (original, non-formatted value which can be useful for number, dates, etc).
@jughosta This is also a reasonable possibility IMO. I think as far as this PR goes, it makes sense to have the initial implementation copy exactly what's seen in the grid (as it now does), since I think that's what most users will be expecting, but it may be worth noting this to have a discussion and potentially opening another issue for the other use case. Power users especially might find the 'copy unformatted value' functionality useful.
@davismcphee Sounds good! Would you suggest to keep "Copy value" label or change to something like "Copy" in preparation for extending the functionality in the future? |
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: cc @jughosta |
@jughosta Personally I prefer |
Closes elastic#108857 ## Summary Before we had "Copy to clipboard" only for cell popovers with JSON content. This PR adds it also for any other cell values. ![Jan-25-2023 18-23-03](https://user-images.githubusercontent.com/1415710/214636400-b347e856-8bf0-4038-bc41-aae23df5e5a9.gif) ### Checklist - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [x] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers)
Closes elastic#108857 ## Summary Before we had "Copy to clipboard" only for cell popovers with JSON content. This PR adds it also for any other cell values. ![Jan-25-2023 18-23-03](https://user-images.githubusercontent.com/1415710/214636400-b347e856-8bf0-4038-bc41-aae23df5e5a9.gif) ### Checklist - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [x] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers)
Closes #108857
Summary
Before we had "Copy to clipboard" only for cell popovers with JSON content. This PR adds it also for any other cell values.
Checklist