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.

Sign up for GitHub

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

Jump to bottom

[DataGrid] Allow to copy the selected rows to the clipboard #1929

Merged
merged 12 commits into from
Jun 29, 2021
Merged

[DataGrid] Allow to copy the selected rows to the clipboard #1929

merged 12 commits into from
Jun 29, 2021

Conversation

m4theushw
Copy link
Member

@m4theushw m4theushw commented Jun 18, 2021 •
edited
Loading

Related to #199. I didn't use "closes" because this issue is much bigger and we can't really implement it now.

Preview: https://deploy-preview-1929--material-ui-x.netlify.app/components/data-grid/demo/

@m4theushw m4theushw added component: data grid This is the name of the generic UI component, not the React module! new feature New feature or request labels Jun 18, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 18, 2021 •
edited
Loading

Is it related to #1197? I had listed a bunch of issues we could solve in isolation.

For instance, CTRL+c that triggers the edit mode? No. Here you go, one good PR: alphabetical characters shouldn't trigger the edit mode if ctrl, meta, or shift are pressed 😁.

@m4theushw
Copy link
Member Author

m4theushw commented Jun 18, 2021 •
edited
Loading

@oliviertassinari Interesting, this issue was never mentioned in #199. This PR is actually an enhancement / bugfix of the current copy feature. If you CTRL + c in a row it selects the text then uses the copy command to write to the clipboard, this is not good. I'm leveraging the Clipboard API now. The old copy doesn't support copying the selected rows, now it does and the fields are exported separated by TAB so it can directly be pasted into Excel. I added support for pressing ALT + c to copy including the headers.

From the issues you mentioned in #1197:

The virtualization is not supported, the whole row isn't copied:

Fixed.

When we use CTRL+c in an editable cell, the edit mode enter, this is very wrong.

Fixed.

In the future, we might see developers asking for: a. customizing the conversion from cell value to text, b. customizing the delimiter, c. asking for imperative copy APIs. We can wait

a. The CSV export is used internatelly, so value formatters are supported
b. No, we use TAB
c. We have an imperative API: copySelectedRowsToClipboard(includeHeaders)

m4theushw
m4theushw commented Jun 18, 2021
View reviewed changes
packages/grid/_modules_/grid/hooks/root/useEvents.ts Outdated
GRID_KEYDOWN,
getHandler(GRID_KEYDOWN),
);

React.useEffect(() => {
if (apiRef.current.rootElementRef?.current) {
Copy link
Member Author

@m4theushw m4theushw Jun 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This effect doesn't work. When it runs, apiRef.current.rootElementRef?.current is null. It's already to be removed by #1862, so a native event listener is used.

oliviertassinari
oliviertassinari reviewed Jun 20, 2021
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Great start, much better than what we had before 👍

  1. If the row selection is empty, should CTRL + c copy the focused cell?
  2. If text is selected natively, inside a read cell, or inside an edit cell, the native CTRL + c behavior is overridden, not great.
  3. I froze my laptop doing a CTRL + c on the 3m cells demo 😆. It froze Google Spreadsheet too. Only Excel got almost decent performance. AG Grid completely goes sideway when it happens, even with the select-all feature 🙈.

packages/grid/_modules_/grid/hooks/features/clipboard/useGridClipboard.ts Show resolved Hide resolved
packages/grid/_modules_/grid/hooks/features/clipboard/useGridClipboard.ts
Comment on lines +51 to +57
if (navigator.clipboard) {
navigator.clipboard.writeText(data).catch(() => {
writeToClipboardPolyfill(data);
});
} else {
writeToClipboardPolyfill(data);
}
Copy link
Member

Choose a reason for hiding this comment

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

In the docs, we use this package: https://github.com/feross/clipboard-copy/blob/master/index.js, in case there is interesting stuff to copy from them. Maybe we could actually make this function in its own file?

packages/grid/_modules_/grid/models/gridExport.ts Outdated Show resolved Hide resolved
packages/grid/_modules_/grid/hooks/features/clipboard/useGridClipboard.ts Outdated Show resolved Hide resolved
m4theushw
m4theushw commented Jun 22, 2021
View reviewed changes
packages/grid/_modules_/grid/hooks/features/rows/useGridParamsApi.ts
const params: GridCellParams = {
...baseParams,
Copy link
Member Author

@m4theushw m4theushw Jun 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I got an improvement from 20ms to 8ms in https://deploy-preview-1929--material-ui-x.netlify.app/components/data-grid/#commercial-version by removing this spread operator.

Copy link
Member

@oliviertassinari oliviertassinari Jun 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Wow, it's pretty wild 👏!

On the full 100,000 rows data set, filtering goes from

Capture d’écran 2021-06-24 à 18 57 06

https://material-ui.com/components/data-grid/#commercial-version

to

Capture d’écran 2021-06-24 à 18 57 12

https://deploy-preview-1929--material-ui-x.netlify.app/components/data-grid/#commercial-version

Sorting is unchanged.

@m4theushw @oliviertassinari
Update packages/grid/_modules_/grid/hooks/features/clipboard/useGridC…
48f575e 
…lipboard.ts

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
@DanailH
Copy link
Member

DanailH commented Jun 24, 2021

Is there a change in the actual functionality, I mean does the docs needs to be updated?

@m4theushw
Copy link
Member Author

Is there a change in the actual functionality, I mean does the docs needs to be updated?

@DanailH The actual functionality was not working very well. The docs in general doesn't need updates. Only the accessibility page was updated to mention the new shortcut to copy including headers: Alt + c.

@m4theushw m4theushw merged commit a3248d1 into mui:master Jun 29, 2021
@m4theushw m4theushw deleted the clipboard branch June 29, 2021 20:35
@m4theushw m4theushw mentioned this pull request Jul 1, 2021
Closed
@oliviertassinari oliviertassinari mentioned this pull request Jul 17, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliviertassinari oliviertassinari oliviertassinari approved these changes

@dtassone dtassone Awaiting requested review from dtassone

@DanailH DanailH Awaiting requested review from DanailH

Assignees
No one assigned
Labels
component: data grid This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@m4theushw @oliviertassinari @DanailH @dtassone