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

[DataGridPremium] Implement Clipboard import #7389

Merged
merged 165 commits into from
May 12, 2023

Conversation

cherniavskii
Copy link
Member

@cherniavskii cherniavskii commented Jan 4, 2023

Closes #199

Preview: https://deploy-preview-7389--material-ui-x.netlify.app/x/react-data-grid/clipboard/#clipboard-paste

Changelog

  • 🎁 Introduce clipboard paste support for DataGridPremium:

    clipboard-paste.mov

    See the documentation for more information

TODO:

  • Copy only a single cell when only one row is selected (experiment)
  • Parse booleans - only paste if the value is boolean?
  • Make this feature disabled by default?
  • Persistence - how to capture pasted data?
    Reuse the same API as for editing - processRowUpdate
  • Move clipboard paste to DataGrid Premium
  • Cmd+V should not enter edit mode http://localhost:3001/x/react-data-grid/editing/#making-a-column-editable
  • Pasting row with country column throws an error http://localhost:3001/x/react-data-grid/filtering/
  • Do not always copy formattedValue to the clipboard - this would probably depend on the column type
    This is still unclear, because usually we want to copy formatted value, but then we have to parse it somehow 🤷🏻‍♂️
  • How to parse custom column types? Maybe add parseClipboardPasteValue to GridColDef?
    valueParser could be used (same as in Editing)
  • Mention valueSetter and valueParser in the docs
  • Parse date fields properly
  • Parse dateTime fields properly
  • Fix NaN after pasting values from the Quantity column: https://deploy-preview-7389--material-ui-x.netlify.app/x/react-data-grid/row-selection/#multiple-row-selection
  • Fix paste not working in Firefox (TypeError: navigator.clipboard.readText is not a function). Try using document.addEventListener('paste') instead
  • Add more tests

@cherniavskii cherniavskii added component: data grid This is the name of the generic UI component, not the React module! new feature New feature or request labels Jan 4, 2023
@mui-bot
Copy link

mui-bot commented Jan 4, 2023

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-7389--material-ui-x.netlify.app/

Updated pages

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 630.5 998.4 841.1 825.76 124.962
Sort 100k rows ms 663.6 1,160.7 1,160.7 982.18 168.871
Select 100k rows ms 298.8 354.6 311 322.38 23.313
Deselect 100k rows ms 174.9 386.3 216.6 257.84 76.485

Generated by 🚫 dangerJS against d1301f3

@joserodolfofreitas
Copy link
Member

joserodolfofreitas commented Jan 5, 2023

Thank you for making the prototype available; It feels quite solid, considering its early stage!
A couple of thoughts:

A. When using Row selection(and Range selection too), I think we should paste the data to the selected rows. So instead of using the focused cell as a starting point, I'd try with the first visible cell on the top-most row (not the first selected necessarily).

pasting.to.selected.rows.mov

B. I think the paste data should not leak out of a row or range selection.
The exceptions are when there's only the cell in focus(no selection), and maybe when it's the last line (for row selection).
In these cases, the current behavior (using the focused cell as a starting point) is fine.
This is the behavior of Apple's Numbers, and I think it's the best (most fine-tuned) experience regarding this aspect.
Additionally, we also need to consider that there's no ctrl+z to this operation, so I think to match the user's expectations, the best bet is to respect the bounds of the selection.

C. There's no way to copy a single selected row now that we're copying the focused cell on ctrl+v.
I agree that in most use cases, users are likely willing to copy a single cell. This can become a pain point, particularly since row selection is enabled by default.
By the way, I'm not sure it should be the default, but maybe we explore this as a connected but separate problem.

Regarding the default copy behaviors, I think we could try the following:

  • When only Row selection is enabled
    Users copy the entire row. Developers can enable "copy from cell" through a secondary property.

  • When only Range selection is enabled
    Users copy the cell if only a single cell is selected (focused).

  • When both selections are enabled (mixed)
    The range selection behavior takes priority. Users copy the cell if only a single cell is selected (focused).

p.s.: AG Grid pastes from the focused cell instead of selected rows (point A.) and leaks the pasted data out of the selection (point B.). But it doesn't feel ideal. We can try improving it.

@joserodolfofreitas
Copy link
Member

A. When using Row selection(and Range selection too), I think we should paste the data to the selected rows. So instead of using the focused cell as a starting point, I'd try with the first visible cell on the top-most row (not the first selected necessarily).

I missed an aspect of my suggestion: with row selection, if we identify that a single cell is going to be pasted, we paste it only to the focused cell.
If it's a range selection, we paste the single value to every cell.

So regarding the default paste behaviors, I think we could try the following:

  • When only Row selection is enabled
    with a single selected row:

    • Single values are pasted to the focused cell.
    • Multiple values are pasted starting from the focused cell.

    with multiple selected rows:

    • Single values are pasted to the focused cell.
      (Perhaps paste to the entire column on the selected rows based on a custom prop like advancedPasting?)
    • Multiple values are pasted starting from the first and top-most cell.
  • When Range selection is enabled:

    • Single values are pasted to all selected cells in the range.
    • Multiple values are pasted starting from the first and top-most cell in the selection.
      (Should they repeat? That's a hard one. Perhaps we enable repeating the pattern across the selected range also based on a custom prop like advancedPasting?)

@github-actions

This comment was marked as outdated.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 13, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 23, 2023
@hamodey
Copy link

hamodey commented May 11, 2023

One more suggestions possibly to handle parsing percentages.

Copying from excel value is 90% this should be converted in the input field as 0.9 or 90% depending on the column type/format.

With a mui Input component setting to type of number will sway the pasted results. So, if data cleaning is needed maybe there could be a prePaste callback that will be called prior to filling the Grid?

This is my outside perspective on the issue so excuse me if this is not the correct place to comment.

@cherniavskii
Copy link
Member Author

Hey @hamodey
If you expect percentage string values to be pasted into a column, you can use pastedValueParser to handle it, here is a demo:
https://codesandbox.io/s/cranky-heisenberg-gd24uo?file=/demo.tsx

Screen.Recording.2023-05-11.at.14.42.37.mov

Parsing different formats of numbers is very tricky - I've described this problem a bit in #7389 (comment)

@DanailH DanailH requested a review from richbustos May 12, 2023 10:52
Copy link
Member

@MBilalShafi MBilalShafi left a comment

Choose a reason for hiding this comment

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

🎉

docs/data/data-grid/clipboard/clipboard.md Outdated Show resolved Hide resolved
* The character used to separate cell values when copying to the clipboard.
* @default '\t'
*/
clipboardCopyCellDelimiter: PropTypes.string,
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: Probably more relatable to unstable_splitClipboardPastedText, also logically speaking we are delimiting the copied text (as a whole) not cells, right?

Suggested change
clipboardCopyCellDelimiter: PropTypes.string,
clipboardCopyTextDelimiter: PropTypes.string,

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this delimiter is only used for separating cell values in the row.
The rows are separated by the newline delimiter. It's not configurable at the moment, but I'm not even sure if anyone would ever want to change this.

Comment on lines 89 to 93
### Format of the pasted data

By default, the clipboard paste expects specific format of the clipboard content: the cell values should be separated by a tab (`\t`) character and the rows should be separated by a new line (`\n`) character.

You can use the `unstable_splitClipboardPastedText` prop to support a different format:
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: Not sure but I'm under the impression that something like delimitClipboardCopiedText instead of clipboardCopyCellDelimiter would be:

  1. More aligned with the unstable_splitClipboardPastedText prop in terms of the interface
  2. Will give slightly more control to the user to also be able to configure row delimiter (let's say \n -> Tab) apart from the cell one (like \t -> ,).

Feel free to drop the suggestion though.

Also, should we cover both aspects (copy and paste delimiters) in this section?

Suggested change
### Format of the pasted data
By default, the clipboard paste expects specific format of the clipboard content: the cell values should be separated by a tab (`\t`) character and the rows should be separated by a new line (`\n`) character.
You can use the `unstable_splitClipboardPastedText` prop to support a different format:
## Format of the clipboard data
By default, the clipboard copy and paste support this format for the clipboard content: the cell values will be separated by a tab (`\t`) character and the rows will be separated by a new line (`\n`) character.
You can use the `clipboardCopyTextDelimiter` prop to change the delimiter for clipboard copy and the `unstable_splitClipboardPastedText` prop to support a different format for clipboard paste

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm under the impression that something like delimitClipboardCopiedText instead of clipboardCopyCellDelimiter would be:
More aligned with the unstable_splitClipboardPastedText prop in terms of the interface

Initially, I wanted to add a callback prop for clipboard data serialization, but this would require deeper changes in the clipboard copy logic since it reuses the getDataAsCsv method that returns a string, not raw data.

We can add it later if necessary, but I would expect that changing the row delimiter (\n) to something else is super rare.

For the clipboard import, the main reason I decided to add the unstable_splitClipboardPastedText callback is to potentially allow users to support both , and \t as cell delimiters.

Copy link
Member Author

Choose a reason for hiding this comment

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

should we cover both aspects (copy and paste delimiters) in this section?

Good idea 👍

docs/data/data-grid/clipboard/clipboard.md Show resolved Hide resolved
docs/data/data-grid/clipboard/clipboard.md Outdated Show resolved Hide resolved
Copy link
Member

@DanailH DanailH left a comment

Choose a reason for hiding this comment

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

Awesome work @cherniavskii 🙌 The feature looks great. I particularly like the demo showcasing what has been copied.
I see that Bilal has left a couple of suggestions but other than that I think we can release it.

Copy link
Member

@DanailH DanailH left a comment

Choose a reason for hiding this comment

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

Awesome work @cherniavskii 🙌 The feature looks great. I particularly like the demo showcasing what has been copied.
I see that Bilal has left a couple of suggestions but other than that I think we can release it.

@cherniavskii cherniavskii changed the title [DataGridPremium] Clipboard import [DataGridPremium] Implement Clipboard import May 12, 2023
@cherniavskii cherniavskii merged commit 5321bd3 into mui:master May 12, 2023
@cherniavskii cherniavskii deleted the clipboard-import branch May 12, 2023 14:28
@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 18, 2023

Awesome feature 🔥


I have noticed 3 things that can be improved on the docs page:

  1. https://mui.com/x/react-data-grid/clipboard/#clipboard-copy. This is flagged as an open-source feature but the demo imports <DataGridPremium>. It sounds like we should split this demo in two to have each plan isolated and clearly documented. Also raised in [data grid] Implement Clipboard import #199 (comment) by a developer of the community.
  2. https://mui.com/x/react-data-grid/clipboard/#format-of-the-clipboard-data is flagged as an open-source but documented after a premium one. It should be moved up, to be documented with the rest of the open source features of the page.
  3. Unless the cell is editable editMode="row"or column with editable: true, when unstable_cellSelection is enabled, seeing the text cursor is confusing: Screenshot 2023-07-18 at 23 30 50

It feels like I can act on this as a text, no I can't.

Screen.Recording.2023-07-18.at.23.34.09.mov

I would expect cursor: default

@oliviertassinari oliviertassinari added the feature: Selection Related to the data grid Selection feature label Jul 18, 2023
@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 19, 2023

Feedback moved to isolated GitHub issues:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! feature: Selection Related to the data grid Selection feature new feature New feature or request plan: Premium Impact at least one Premium user v6.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[data grid] Implement Clipboard import
8 participants