elastic / eui

Elastic UI Framework 🙌
https://eui.elastic.co/
Other
54 stars 841 forks source link

[EuiDataGrid] Add support for copying the focused cell's value with keyboard shortcut only #6561

Open davismcphee opened 1 year ago

davismcphee commented 1 year ago

We're working on improving copy/paste support for the Discover data grid (https://github.com/elastic/kibana/pull/149525), and we think there's an opportunity to improve general support for it in the EuiDataGrid.

Rather than the user having to manually highlight the entirety of the cell value to copy it, or expanding the cell and highlighting the popover value, both of which can be difficult (especially when there are cell actions or spaces in the text), it would be convenient to offer built-in support for copying the focused cell's value. I believe this should be possible by hooking into the browser's copy event: https://developer.mozilla.org/en-US/docs/Web/API/Element/copy_event.

Two notes about this request:

Here's a 10-minute hack job I threw together to demonstrate the concept that can be pasted into the EuiDataGrid docs page or anywhere else containing an EuiDataGrid:

document.addEventListener('copy', (e) => {
  if (document.activeElement.classList.contains('euiDataGridRowCell') && !document.getSelection().toString()) {
    const text = document.activeElement.querySelector('[data-datagrid-cellcontent]').innerText;
    event.clipboardData.setData('text/plain', text);
    event.preventDefault();
    console.log(text);
  }
});
cee-chen commented 1 year ago

Hi @davismcphee! We discussed this in our EUI sync today and being able to Ctrl/Cmd+C to copy a cell's value is definitely a cool concept we'd like to support and add to the keyboard shortcuts menu. I strongly agree with your proposal of an onCellCopy API vs us trying to detect cell content via scraping the DOM (although we can certainly attempt to fall back to innerText) - the consumer knows their own data much better than EUI does and is in a better position to customize that for copy purposes.

As a heads up however, it's currently a medium priority for us, unless you have a use case for multiple teams/products that would bump it to a high. This means we don't currently have an ETA for this - but if you're interested in open source contributing to EuiDataGrid, we'd definitely take a PR on this!

davismcphee commented 1 year ago

@cee-chen Thanks for the update, and I'm glad to hear the EUI team agrees that it would be a useful addition! As of right now we don't have an urgent need for the functionality since we at least have a workaround implemented in Discover using a Copy value action, so not giving it a high a priority is totally fine for us. But I'll keep this in mind next time I have some time to work on an EUI contribution and may take it on myself if it's still open 🙂.

jughosta commented 1 year ago

We have complains from users:

https://discuss.elastic.co/t/copy-paste-from-kibana-without-row-and-column-information/320048/ https://discuss.elastic.co/t/copy-paste-from-kibana-without-row-and-column-information-2/334026

They would like to copy values of selected cells. And not only on Discover page.

cee-chen commented 1 year ago

@jughosta it's not clear to me from those threads whether or not those users are complaining about the ability to use a keyboard shortcut to copy the text, or if said data grid implementations are simply missing a clickable copy cell action entirely.

This issue is for the former (cmd/ctrl+C to copy) - the latter is up to your team to implement.

jughosta commented 1 year ago

@cee-chen Sorry, it might be not related to the Davis' request and we should open another issue. But it's related to the grid :)

My guess is that users would like to copy some selection of cells contents as from any other web page. They can do that currently but the result is not expected. They get extra text (probably a11y labels too) instead of the plain selection. It looks like the following:

8c713bbeed895edd09f484123b5d0af22fd81596

Would it be possible to skip extra wording (like columns, rows, etc) from a natively copied text?

cee-chen commented 1 year ago

Ahhh gotcha. That text is coming from our screen reader text that describes each row. We can certainly figure out a way to make it non-selectable or not show up while being copied and pasted. Let's open up a separate issue for that however.

github-actions[bot] commented 12 months ago

👋 Hi there - this issue hasn't had any activity in 6 months. If the EUI team has not explicitly expressed that this is something on our roadmap, it's unlikely that we'll pick this issue up. We would sincerely appreciate a PR/community contribution if this is something that matters to you! If not, and there is no further activity on this issue for another 6 months (i.e. it's stale for over a year), the issue will be auto-closed.

davismcphee commented 7 months ago

Just bumping this one to prevent staleness since it's still a valuable feature to have 🙂

cee-chen commented 7 months ago

Totally agreed - any chance you'd be interested in contributing the feature to EUI? 😄

davismcphee commented 7 months ago

I'm definitely interested! Just not sure yet if I'll be able to find time to work on it near term 😅

cee-chen commented 7 months ago

All good! I know that feeling hahaa 🫠

JasonStoltz commented 5 months ago

@cee-chen We have this labeled as a "Large" effort. Do you still think this is a large amount of work?

cee-chen commented 5 months ago

I think anything involving EuiDataGrid is a large effort 😅 But yes, since I think the API on this one will be fairly complex since it will involve adding a new prop allowing consumers to pass us copyable content and falling back to inner text if not.

cee-chen commented 1 month ago

@davismcphee Our stalebot went rogue and closed this unintentionally - but just curious, would you consider this feature request moot since copying cells directly now works as of #8019? Can we go ahead and leave it closed?

davismcphee commented 3 weeks ago

@cee-chen Thanks for the ping. While #8019 is a great improvement, personally I think this is still a valuable feature for easily copying entire cell values, vs having to manually highlight the text to copy. It seems especially useful for cells with truncated content where it's trickier to select the entire value, and for keyboard users. It's also a common pattern in other data grids I've used, and we've encountered users in Discover with similar expectations.