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

[data grid] Pressing Delete key in a Boolean type cell, the value incorrectly resets to empty string #12125

Closed
mororyou opened this issue Feb 19, 2024 · 6 comments · Fixed by #12216
Assignees
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 good first issue Great for first contributions. Enable to learn the contribution process. ready to take Help wanted. Guidance available. There is a high chance the change will be accepted

Comments

@mororyou
Copy link

mororyou commented Feb 19, 2024

Steps to reproduce

Link to live example: https://codesandbox.io/p/github/mororyou/mui-datagrid-test/main

  1. To open Developer Tools

  2. Hover over the cells in the isAdmin column.

  3. To delete, use the Delete key or the Backspace key.

  4. Check the Console in Developer Tools.

  5. You can confirm in the console that the value of valueSetter's params is an empty string.

Also, I'll attach a screenshot for reference.
https://gyazo.com/787beae18b71f4366be716d654a6cf50

Current behavior

I was expecting to receive a Boolean, but an empty string is being sent.

Expected behavior

Can we change the Boolen value so that it can be retrieved?

Context

No response

Your environment

npx @mui/envinfo
  Don't forget to mention which browser you used.
  Output from `npx @mui/envinfo` goes here.

Search keywords: DataGrid, valueSetter
Order ID: 67619

@mororyou mororyou added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Feb 19, 2024
@MBilalShafi MBilalShafi added the component: data grid This is the name of the generic UI component, not the React module! label Feb 19, 2024
@michelengelen
Copy link
Member

michelengelen commented Feb 19, 2024

Hey @mororyou and thanks for raising this.
I can confirm that this is a bug (or at least an inconsistency) that we can improve upon.

I did play around with it a bit and found an easy way to fix this for booleans and potentially all other values as well (if we do decide to introduce default values for those:

diff --git a/packages/x-data-grid/src/hooks/features/editing/useGridCellEditing.ts b/packages/x-data-grid/src/hooks/features/editing/useGridCellEditing.ts
index 25c7cfdb5..3108844a7 100644
--- a/packages/x-data-grid/src/hooks/features/editing/useGridCellEditing.ts
+++ b/packages/x-data-grid/src/hooks/features/editing/useGridCellEditing.ts
@@ -329,8 +329,24 @@ export const useGridCellEditing = (
       const { id, field, deleteValue, initialValue } = params;

       let newValue = apiRef.current.getCellValue(id, field);
-      if (deleteValue || initialValue) {
-        newValue = deleteValue ? '' : initialValue;
+
+      if (deleteValue) {
+        const fieldType = apiRef.current.getColumn(field).type;
+        switch (fieldType) {
+          case 'boolean':
+            newValue = false;
+            break;
+          case 'date':
+          case 'dateTime':
+          case 'number':
+            newValue = undefined;
+            break;
+          case 'singleSelect':
+          case 'string':
+          default:
+            newValue = '';
+            break;
+        }
+      } else if (initialValue) {
+        newValue = initialValue;
       }

       const newProps = {

It would be beneficial if we could add a test or two as well to prevent regressions.
Since this is a fairly easy task to do I will also open this up as a good first issue.

Thanks again for reporting it! 🙇🏼

@michelengelen michelengelen added bug 🐛 Something doesn't work good first issue Great for first contributions. Enable to learn the contribution process. ready to take Help wanted. Guidance available. There is a high chance the change will be accepted feature: Editing Related to the data grid Editing feature and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Feb 19, 2024
@michelengelen michelengelen changed the title When pressing the Delete key in a Boolean type cell, the params.value of the valueSetter function becomes an empty string [data grid] Pressing Delete key in a Boolean type cell, the value incorrectly resets to empty string Feb 19, 2024
@sooster910
Copy link
Contributor

sooster910 commented Feb 19, 2024

hi @michelengelen, can I work on this issue? Kindly assign it to me! 🙏

@LumaKernel
Copy link

@michelengelen

+          case 'singleSelect':
+          case 'string':
+          default:
+            newValue = '';
+            break;

For singleSelect, is it also intended to fallback into empty string?

@sooster910
Copy link
Contributor

sooster910 commented Feb 22, 2024

I've been thinking about the suggestion to set the singleSelect field's value to an empty string ('') upon a delete key event.

From what I've gathered, it might enhance user experience to more explicitly indicate a 'no selection' state with null or undefined for singleSelect fields. This seems like it could more clearly align with a user's intent to 'unselect' and help in maintaining data consistency.

Since I'm new and still learning about the project's data handling, I would greatly appreciate any additional insights on this matter. 😊

@michelengelen
Copy link
Member

That sounds about right. I just left it as an empty string because the previous implementation was using it as well ...

  if (deleteValue || initialValue) {
    newValue = deleteValue ? '' : initialValue;
  ...

I would be fine with null

Copy link

⚠️ This issue has been closed.
If you have a similar problem, please open a new issue and provide details about your specific problem.
If you can provide additional information related to this topic that could help future readers, please feel free to leave a comment.

How did we do @mororyou?
Your experience with our support team matters to us. If you have a moment, please share your thoughts through our brief survey.

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 good first issue Great for first contributions. Enable to learn the contribution process. ready to take Help wanted. Guidance available. There is a high chance the change will be accepted
Projects
None yet
5 participants