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 calling onCellEditStop on error #12747

Merged
merged 6 commits into from Apr 15, 2024

Conversation

sai6855
Copy link
Contributor

@sai6855 sai6855 commented Apr 11, 2024

closes: #11406

@sai6855 sai6855 added 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 labels Apr 11, 2024
@sai6855 sai6855 marked this pull request as draft April 11, 2024 13:56
@mui-bot
Copy link

mui-bot commented Apr 11, 2024

Deploy preview: https://deploy-preview-12747--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against a887256

@sai6855 sai6855 changed the title [DataGrid] Fix cell editing bug in useGridCellEditing hook [DataGrid] Fix calling onCellEditStop on error Apr 11, 2024
@sai6855 sai6855 marked this pull request as ready for review April 11, 2024 14:18
Comment on lines 241 to 260
const { field, id, value, debounceMs, unstable_skipValueParser: skipValueParser } = args[0];
const column = apiRef.current.getColumn(field);
const row = apiRef.current.getRow(id)!;

let parsedValue = value;
if (column.valueParser && !skipValueParser) {
parsedValue = column.valueParser(value, row, column, apiRef);
}

const editingState = gridEditRowsStateSelector(apiRef.current.state);
let newProps: GridEditCellProps = {
...editingState[id][field],
value: parsedValue,
changeReason: debounceMs ? 'debouncedSetEditCellValue' : 'setEditCellValue',
};

if (column.preProcessEditCellProps) {
const hasChanged = value !== editingState[id][field].value;

newProps = { ...newProps, isProcessingProps: true };
Copy link
Contributor Author

@sai6855 sai6855 Apr 11, 2024

Choose a reason for hiding this comment

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

This entire logic was copied from

const { id, field, value, debounceMs, unstable_skipValueParser: skipValueParser } = params;
throwIfNotEditable(id, field);
throwIfNotInMode(id, field, GridCellModes.Edit);
const column = apiRef.current.getColumn(field);
const row = apiRef.current.getRow(id)!;
let parsedValue = value;
if (column.valueParser && !skipValueParser) {
parsedValue = column.valueParser(value, row, column, apiRef);
}
let editingState = gridEditRowsStateSelector(apiRef.current.state);
let newProps: GridEditCellProps = {
...editingState[id][field],
value: parsedValue,
changeReason: debounceMs ? 'debouncedSetEditCellValue' : 'setEditCellValue',
};
if (column.preProcessEditCellProps) {
const hasChanged = value !== editingState[id][field].value;
newProps = { ...newProps, isProcessingProps: true };
updateOrDeleteFieldState(id, field, newProps);
newProps = await Promise.resolve(
column.preProcessEditCellProps({ id, row, props: newProps, hasChanged }),
);
}

Thought of storing { id, row, props: newProps, hasChanged } in ref, which are computed in below function, but wasn't sure if setCellEditingEditCellValue runs always and runs before runIfNoPreProcessError

const setCellEditingEditCellValue = React.useCallback<

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!
Did you consider following the same approach as in #11383

Copy link
Contributor Author

@sai6855 sai6855 Apr 15, 2024

Choose a reason for hiding this comment

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

Wasn't aware of that PR. Now I've updated code to use similar approach as in #11383.

Copy link
Member

@cherniavskii cherniavskii left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@@ -234,6 +234,19 @@ export const useGridCellEditing = (
[apiRef],
);

const runIfNoFieldErrors =
<Args extends any[]>(callback?: (...args: Args) => void) =>
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: better type safety

Suggested change
<Args extends any[]>(callback?: (...args: Args) => void) =>
<Args extends Parameters<GridEventListener<'cellEditStop'>>>(callback?: (...args: Args) => void) =>

@cherniavskii cherniavskii added the CLA: signed See https://www.notion.so/mui-org/CLA-Contributor-License-Agreement-92ece655b1584b10b00e4de9e67eedb0 label Apr 15, 2024
…ting.ts

Signed-off-by: Andrew Cherniavskii <andrew.cherniavskii@gmail.com>
@cherniavskii cherniavskii added needs cherry-pick The PR should be cherry-picked to master after merge and removed needs cherry-pick The PR should be cherry-picked to master after merge labels Apr 15, 2024
@cherniavskii cherniavskii enabled auto-merge (squash) April 15, 2024 12:50
auto-merge was automatically disabled April 15, 2024 13:13

Head branch was pushed to by a user without write access

@romgrk romgrk enabled auto-merge (squash) April 15, 2024 13:24
@romgrk romgrk merged commit 559ae09 into mui:master Apr 15, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work CLA: signed See https://www.notion.so/mui-org/CLA-Contributor-License-Agreement-92ece655b1584b10b00e4de9e67eedb0 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.

[data grid] onCellEditStop is called when cell has error
4 participants