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] valueFormatter doesn't show the updated value after processRowUpdate #11486

Closed
atsoy opened this issue Dec 22, 2023 · 22 comments
Closed
Labels
component: data grid This is the name of the generic UI component, not the React module! support: pro standard Support request from a Pro standard plan user. https://mui.com/legal/technical-support-sla/ support: question Community support but can be turned into an improvement

Comments

@atsoy
Copy link

atsoy commented Dec 22, 2023

Steps to reproduce

Link to live example: (required): https://codesandbox.io/p/sandbox/dgp-cellupdate-bug-rzkwcr

Steps:

  1. Edit Amount column in the 4th row
  2. Optional: Edit Price column the 4th row

Current behavior

total column does not display updated value (product of amount and price).

As soon as another row's cell get an updated value, then previous edited row's total shows correct value

Expected behavior

total column shows updated value, after one of the columns amount || price were updated.

Context

Idea ist to update the total column's cell in the same row, which multiplies amount and price columns, after cell editing ist finished.

After some analysis I've noticed, that valueFormatter actually does get the updated value and formats it correct, but after that it's being called again, but with old value.

P.S: It worked before, but since we've updated x-data-grid-pro to the 6.18.3 it was broken (also tested with 6.18.5)

Your environment

npx @mui/envinfo
  System:
    OS: macOS 13.6.2
  Binaries:
    Node: 18.13.0
    Yarn: 1.22.19
    npm: 8.19.3
  Browsers:
    Chrome: 119.0.6045.159
    Safari: 17.1.2
  npmPackages:
    @emotion/react: ~11.11.1 => 11.11.1 
    @emotion/styled: ~11.11.0 => 11.11.0 
    @mui/base:  5.0.0-beta.27 
    @mui/core-downloads-tracker:  5.14.7 
    @mui/material: ~5.14.7 => 5.14.7 
    @mui/private-theming:  5.14.7 
    @mui/styled-engine:  5.14.7 
    @mui/system:  5.14.7 
    @mui/types:  7.2.4 
    @mui/utils:  5.15.0 
    @mui/x-data-grid:  6.18.5 
    @mui/x-data-grid-pro: ~6.18.3 => 6.18.5 
    @mui/x-date-pickers:  6.18.5 
    @mui/x-date-pickers-pro: ~6.18.3 => 6.18.5 
    @mui/x-license-pro:  6.10.2 
    @types/react: 18.2.24 => 18.2.24 
    react: ~18.2.0 => 18.2.0 
    react-dom: ~18.2.0 => 18.2.0 
    typescript: 5.2.2 => 5.2.2 

Search keywords: datagridpro processrowupdate valueformatter
Order ID: 62555

@atsoy atsoy added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Dec 22, 2023
@zannager zannager added component: data grid This is the name of the generic UI component, not the React module! support: pro standard Support request from a Pro standard plan user. https://mui.com/legal/technical-support-sla/ labels Dec 22, 2023
@DanailH
Copy link
Member

DanailH commented Dec 29, 2023

@atsoy thanks for reporting this. The issue is with the return value of the 'handleProcessRowUpdate' - it should be an array with the mutated row objects. You can see it working here: https://codesandbox.io/p/sandbox/dgp-cellupdate-bug-forked-2slznj?file=%2Fsrc%2Fdemo.tsx%3A110%2C32

@DanailH DanailH closed this as completed Dec 29, 2023
@DanailH DanailH added support: question Community support but can be turned into an improvement and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Dec 29, 2023
@atsoy
Copy link
Author

atsoy commented Jan 8, 2024

Thanks @DanailH and HNY.
In the provided example Typescript complains about types for processRowUpdate, since it expects just a R.

Was already surprised, cause we've used to return a single row, not an array.

@DanailH
Copy link
Member

DanailH commented Jan 8, 2024

@atsoy I check it again, there indeed might be a problem with the valueFormatter in combination with processRowUpdate. I'll investigate a bit more and see what I can find.

@DanailH DanailH reopened this Jan 8, 2024
@DanailH
Copy link
Member

DanailH commented Jan 8, 2024

Why do you need to keep your rows in the state? This seems to be causing the problem. if you check this example the issue will be more visible: https://mui.com/x/react-data-grid/editing/#server-side-persistence

@DanailH DanailH added the status: waiting for author Issue with insufficient information label Jan 8, 2024
@atsoy
Copy link
Author

atsoy commented Jan 8, 2024

Thanks for checking @DanailH !

The example I've provided based on local state, in reality we use reducer (useReducer), where our data is being stored. Users can manipulate table data and then send it to the server (not after each edit stop, but form like - pressing on submit button).

I'm not sure I understand the view point of "storing data in state seems to be causing the issue". Updated data is being set properly. Please elaborate.

P.S: The process of updating the rows, formatting updated values etc worked without issues by using processRowUpdate and valueFormatter until we've upgraded the version to ~6.18.3 (previous version was ~6.12.1) Today tested with 6.18.7

@github-actions github-actions bot added status: waiting for maintainer These issues haven't been looked at yet by a maintainer and removed status: waiting for author Issue with insufficient information labels Jan 8, 2024
@michelengelen michelengelen changed the title [DataGridPro]: valueFormatter doesn't show the updated value after processRowUpdate [data grid] valueFormatter doesn't show the updated value after processRowUpdate Jan 15, 2024
@michelengelen
Copy link
Member

@DanailH gentle ping! 🙇🏼

@atsoy
Copy link
Author

atsoy commented Jan 23, 2024

Hi @michelengelen @DanailH , I understand that you're working on this project voluntarily, and I truly appreciate the time and effort you invest in it. If possible, could you provide an ETA for when this issue might be fixed?

Thank you once again for your dedication to the project!

@DanailH
Copy link
Member

DanailH commented Jan 24, 2024

@atsoy I'm sorry for the delay. I checked it again and the issue is that handleProcessRowUpdate expects a return value - the updated row. As far as I can tell you are updating your state that contains the rows and then expect the grid to rerender with the new values which doesn't happen. You need to return the updated value of the row you are editing with the handleProcessRowUpdate.

I've updated the example again https://codesandbox.io/p/sandbox/dgp-cellupdate-bug-forked-9kxt24

@DanailH DanailH added status: waiting for author Issue with insufficient information and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jan 24, 2024
@atsoy
Copy link
Author

atsoy commented Jan 24, 2024

@DanailH thanks for coming back!
I actually do not really understand, why should I manipulate the newRow to display updated product of 2 fields. I mean in the example I've provided - it was simplified by using useState, in reality we use useReducer.

I've updated provided example.

As far as I can tell you are updating your state that contains the rows and then expect the grid to rerender with the new values which doesn't happen.

It actually happens, but after that, I assume grid re-renders after some effect and restores probably from internal grid state.

@github-actions github-actions bot added status: waiting for maintainer These issues haven't been looked at yet by a maintainer and removed status: waiting for author Issue with insufficient information labels Jan 24, 2024
@DanailH
Copy link
Member

DanailH commented Jan 24, 2024

@mui/xgrid can someone take a look? It might be that I don't understand the issue.

@DanailH DanailH removed their assignment Jan 24, 2024
@cherniavskii
Copy link
Member

This looks like a regression introduced in 6.18.3
v6.18.2: https://codesandbox.io/p/sandbox/dgp-cellupdate-bug-forked-4skj6q
v6.18.3: https://codesandbox.io/p/sandbox/dgp-cellupdate-bug-forked-mqfp7x

@cherniavskii
Copy link
Member

While this looked like a regression at first glance, I'm not sure about this now.
I've created a simplified demo with a delay after updating the rows' state.
With the delay, the issue is reproducible in both 6.18.2 and 6.18.3:
v6.18.2: https://codesandbox.io/p/sandbox/dgp-cellupdate-bug-forked-qf8769
v6.18.3: https://codesandbox.io/p/sandbox/dgp-cellupdate-bug-forked-vf2zn7

Added delay makes it clear that it's a race condition - the rows state gets updated with a new total field value, but the newRow returned from processRowUpdate has the old total value.
Before 6.18.3, the rows state update takes precedence over newRow, but starting from 6.18.3 the newRow it's not the case anymore.

@atsoy Before we proceed with further investigation, did you consider using valueGetter to derive the total column? Here's a demo: https://codesandbox.io/p/sandbox/dgp-cellupdate-bug-forked-wy8qgp

@atsoy
Copy link
Author

atsoy commented Jan 25, 2024

@cherniavskii thanks for your input!

Before we proceed with further investigation, did you consider using valueGetter to derive the total column? Here's a demo: https://codesandbox.io/p/sandbox/dgp-cellupdate-bug-forked-wy8qgp

Yes, I'll try to use that as a workaround and will give an update

P.S: I'm not sure if it's the right place, but did you or do you consider to provide e.g an option / method to access the current grid data as it is (since there are already possibilities to export CSV or printable data).

@cherniavskii
Copy link
Member

do you consider to provide e.g an option / method to access the current grid data as it is

The same data that you pass to the grid through the rows prop? What is the use case for this?

@atsoy
Copy link
Author

atsoy commented Jan 25, 2024

Sorry, I mean of course not the same data, which is provided to rows, but the current State of it (e.g after some data manipulations).

Use case e.g to get avoid usage of useState and useReducer and get the data direct out of grid (let's say for submitting a form with some other fields and grid data).

@atsoy
Copy link
Author

atsoy commented Feb 6, 2024

Hey @cherniavskii , thanks again for the provided workaround idea. I've implemented it in multiple tables. However I think I found another bug, which is be related to the existing one (or may be have same source):

In case of the let's say price field i expect the input like 2,33 or 2.33, which will be validated, parsed and stored as 2.33 in the reducer state. In case user types 2,33 (with comma), it renders NaN in the end, but should actually show 2.33 (with dot).

I didn't add the preProcessEditProps validation, but we have one, which checks the input and shows error if something is wrong (as a tooltip).

See example

If you uncomment (valueGetter) it will work fine, but still not the desired functionality (imo), because of provided data, which should be updated in the internal grid state as well and rendered in cells.
Or I might understand the grid component design and purpose wrong..

@zannager zannager removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Feb 15, 2024
@atsoy
Copy link
Author

atsoy commented Feb 15, 2024

to follow up: will this be considered as a bug or may be analized? I ask because of this statement.

Before 6.18.3, the rows state update takes precedence over newRow, but starting from 6.18.3 the newRow it's not the case anymore.

@cherniavskii
Copy link
Member

cherniavskii commented Feb 21, 2024

Hey @atsoy
Sorry for the late reply!

I've looked into the codesandbox you provided.
First of all, the reason you see NaN after editing is that you use the string column type (it's a default) for numeric data, this is why the string is not parsed into the number properly. Adding type: "number" solves the issue.
Then, I uncommented the valueGetter for the total column since you want to derive the value for this column from other fields of the row.
Here's a working demo: https://codesandbox.io/p/sandbox/dgp-cellupdate-bug-forked-93cfzv?file=%2Fsrc%2Fdemo.tsx
Note, that the row data passed to processRowUpdate is now properly parsed.
Is this the desired result?

@cherniavskii
Copy link
Member

I mean of course not the same data, which is provided to rows, but the current State of it (e.g after some data manipulations).

Not sure if you're still going to need it, but you can get a single row using API object - apiRef.current.getRow(id).
To get all row data, you can use gridRowsLookupSelector(apiRef) to get the rows lookup and then convert it to an array.
Hope this helps!

@cherniavskii cherniavskii added the status: waiting for author Issue with insufficient information label Feb 22, 2024
Copy link

The issue has been inactive for 7 days and has been automatically closed.

@atsoy
Copy link
Author

atsoy commented Feb 29, 2024

Hi @cherniavskii thanks for explanation. I think this cannot be considered as a bug. So I just comment it and will close this issue.

Again, thanks for the efforts!

@github-actions github-actions bot added status: waiting for maintainer These issues haven't been looked at yet by a maintainer and removed status: waiting for author Issue with insufficient information labels Feb 29, 2024
@github-actions github-actions bot reopened this Feb 29, 2024
@atsoy atsoy closed this as completed Feb 29, 2024
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 @atsoy?
Your experience with our support team matters to us. If you have a moment, please share your thoughts through our brief survey.

@github-actions github-actions bot removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Feb 29, 2024
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! support: pro standard Support request from a Pro standard plan user. https://mui.com/legal/technical-support-sla/ support: question Community support but can be turned into an improvement
Projects
None yet
Development

No branches or pull requests

5 participants