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

[DataGrid] Fix start editing detection of uppercase letter #5405

Merged
merged 2 commits into from
Jul 8, 2022

Conversation

alexfauquette
Copy link
Member

@alexfauquette alexfauquette commented Jul 6, 2022

@alexfauquette alexfauquette added the feature: Editing Related to the data grid Editing feature label Jul 6, 2022
@alexfauquette alexfauquette self-assigned this Jul 6, 2022
@mui-bot
Copy link

mui-bot commented Jul 6, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 240.2 481.4 329.5 337.98 91.644
Sort 100k rows ms 427.1 793.6 719.9 675.64 127.992
Select 100k rows ms 107.1 195.6 177.1 161.38 33.918
Deselect 100k rows ms 87.1 294.9 199.8 183.34 67.899

Generated by 🚫 dangerJS against 64289f2

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.

I guess this is mostly if you want to start with a capital letter. 👍

@alexfauquette alexfauquette merged commit ce80a00 into mui:master Jul 8, 2022
@alexfauquette alexfauquette deleted the fix-maj-editing branch July 8, 2022 12:47
if (event.shiftKey || event.ctrlKey || event.metaKey || event.altKey) {
if (
(event.ctrlKey && event.key !== 'v') ||
(event.metaKey && event.key !== 'v') ||
Copy link
Member

Choose a reason for hiding this comment

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

It reminds me a bit of #3660 (comment) by @cherniavskii. We might want to change how we detect that the key v is pressed.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 8, 2022

@alexfauquette I feel that we are introducing a regression in terms of UX cc @gerdadesign. See the current behavior:

Screen.Recording.2022-07-08.at.18.24.48.mov

https://deploy-preview-5405--material-ui-x.netlify.app/x/react-data-grid/editing/#making-a-column-editable

But in all the other data grids I can benchmark against, Ctrl + V edits the cell without entering in edit mode. You can reproduce this on: Notion, Excel, Google Spreadsheet, AG Grid, and Airtable. It breaks the learnings end-users have built on these tools.

Related to #199 and #4687.

How about we revert this behavior change?

@oliviertassinari oliviertassinari added the component: data grid This is the name of the generic UI component, not the React module! label Jul 8, 2022
@alexfauquette
Copy link
Member Author

@oliviertassinari No problem for removing it. I added the Ctrl + V because I felt it would be a quick win to be able to past something in a cell without having to start edit mode but it can wait the implementation of the clipboard feature

@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Jul 9, 2022
@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 9, 2022

On a related note, ⌥ (option) + p that prints π does not work on this PR as well as before this PR, nor on AG Grid, but it works on Excel.

Maybe this would do it:

diff --git a/packages/grid/x-data-grid/src/hooks/features/editRows/useGridCellEditing.new.ts b/packages/grid/x-data-grid/src/hooks/features/editRows/useGridCellEditing.new.ts
index 243ce0d8..8e984af2 100644
--- a/packages/grid/x-data-grid/src/hooks/features/editRows/useGridCellEditing.new.ts
+++ b/packages/grid/x-data-grid/src/hooks/features/editRows/useGridCellEditing.new.ts
@@ -152,11 +152,7 @@ export const useGridCellEditing = (
         let reason: GridCellEditStartReasons | undefined;

         if (isPrintableKey(event.key)) {
-          if (
-            (event.ctrlKey && event.key !== 'v') ||
-            (event.metaKey && event.key !== 'v') ||
-            event.altKey
-          ) {
+          if (event.ctrlKey || event.metaKey) {
             return;
           }
           reason = GridCellEditStartReasons.printableKeyDown;

I guess this is the kind of logic that is painful to get right, full of hedge cases. So +1 to add a test for event.altKey = true, and event.key = 'π' that is option + P on macOS.

@oliviertassinari oliviertassinari changed the title [DataGrid] Start editing with uppercase letter and Ctrl+V [DataGrid] Fix start editing detection of uppercase letter Jul 9, 2022
@alexfauquette
Copy link
Member Author

@m4theushw Do you know what is the original reason of this condition?

The only problematic key combo I can imagine is Ctrl+C because when user select a box content and copy it, they don't want to go to edit mode.

Otheriwise mot of the combinations I know are to write spécial letters which could start the édit mode. For the Shift we should keep ignoring it. For editing with capital letters but also for users who do not have a num pad and try to edit number columns

@m4theushw
Copy link
Member

m4theushw commented Jul 11, 2022

Do you know what is the original reason of this condition?

@alexfauquette I think that the condition to not start the edit mode was added mainly because of Ctrl + a. But then it also blocked other key combinations.

But in all the other data grids I can benchmark against, Ctrl + V edits the cell without entering in edit mode. You can reproduce this on: Notion, Excel, Google Spreadsheet, AG Grid, and Airtable. It breaks the learnings end-users have built on these tools.

@oliviertassinari If we allow users to paste any content without entering the edit mode, then we won't be able to validate the new data entered which may lead to inconsistencies. For instance, AG Grid has this problem too, if I try to copy from the Name column and paste into the Country column, the content gets messed up because it doesn't have the country flag to show.


Side-note: https://deploy-preview-5405--material-ui-x.netlify.app/x/react-data-grid/#mit-version is still using the legacy API.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 11, 2022

we won't be able to validate the new data entered which may lead to inconsistencies.

@m4theushw Interesting.

My issue as an end-user is that in the very large majority of the cases when I use Ctrl+V, I don't want to further edit the value. Having to systematically press Enter to persist the change is a bit painful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! feature: Editing Related to the data grid Editing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot start editing with Shift+[key] while using newEditingApi?
5 participants