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.

Sign up for GitHub

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

Jump to bottom

[test] Test the validation before saving a value #2087

Merged

m4theushw merged 6 commits into mui:master from m4theushw:cell-editing-validation

Jul 13, 2021

Member

m4theushw commented Jul 6, 2021 •

edited

I just noticed that when calling commitCellChange we're not checking if there's an error. This could cause the invalid value being commited when it shouldn't. This situation may occur with custom edit components.

m4theushw added component: data grid test labels

oliviertassinari reviewed

View reviewed changes

Member

oliviertassinari left a comment

Should GRID_CELL_EDIT_PROPS_CHANGE_COMMITTED fire if there is an error or not? The name of the event is weird. It says committed even if it wasn't yet or might not be.

packages/grid/x-grid/src/tests/editRows.XGrid.test.tsx Outdated Show resolved Hide resolved

m4theushw added 2 commits

July 7, 2021 11:14


          [test] Test the validation before saving a value

069d382


          do not publish GRID_CELL_EDIT_PROPS_CHANGE_COMMITTED if there is an e…

ed7d333

…rror

m4theushw force-pushed the cell-editing-validation branch from 70cadf6 to ed7d333 Compare

July 7, 2021 14:21

Member Author

m4theushw commented Jul 7, 2021 •

edited

Should GRID_CELL_EDIT_PROPS_CHANGE_COMMITTED fire if there is an error or not? The name of the event is weird. It says committed even if it wasn't yet or might not be.

@oliviertassinari True, GRID_CELL_EDIT_PROPS_CHANGE_COMMITTED shouldn't be fired. I pushed one more commit fixing this and also allowing developers to stop the propagation of this event and GRID_CELL_EDIT_PROPS_CHANGE .

In #1955 I changed all edit components to call the API directly, but by doing that the events would not be published and developers couldn't stop the propagation. It also broke https://master--material-ui-x.netlify.app/storybook/?path=/story/x-grid-tests-rows--validate-edit-value-server-side&globals=measureEnabled:false. To fix this, I updated commitCellChange and setEditCellProps and introduced another changeCellEditProps method. The purpose of each one is as following:

changeCellEditProps a syntactic sugar to publish GRID_CELL_EDIT_PROPS_CHANGE
setEditCellProps updates the edit props, subscribes to the GRID_CELL_EDIT_PROPS_CHANGE
commitCellChange checks if there's an error and, if it doesn't, then publishes GRID_CELL_EDIT_PROPS_CHANGE_COMMITTED. The commit logic was moved to a handler that subscribes to this event, so developers can stop it.

We can open another PR to rename GRID_CELL_EDIT_PROPS_CHANGE_COMMITTED and the prop to follow our convention. I was thinking about GRID_CELL_EDIT_COMMIT, because we only commit the value, not all props. What do you think? There's also an inconsistency surrounding EDIT_CELL and CELL_EDIT.

m4theushw requested a review from a team

July 7, 2021 14:40


          introduce changeCellEditProps

b356a75

m4theushw force-pushed the cell-editing-validation branch from 572dd69 to b356a75 Compare

July 7, 2021 18:23

m4theushw mentioned this pull request

[DataGrid] Add row editing feature #2098

Merged

4 tasks

dtassone reviewed

View reviewed changes

packages/grid/_modules_/grid/hooks/features/rows/useGridEditRows.ts Outdated

@@ @@ -137,14 +130,15 @@ export function useGridEditRows(apiRef: GridApiRef) { @@
                   [options.isCellEditable],
                 );
-                const setEditCellProps = React.useCallback(
+                const changeCellEditProps = React.useCallback(

Member

dtassone Jul 8, 2021

setEditCellProps, changeCellEditProps => pretty confusing

Member Author

m4theushw Jul 8, 2021

Yeah, the names are a bit weird, we can discuss. Before #1824, some components were directly publishing GRID_CELL_EDIT_PROPS_CHANGE, but that is not good because TypeScript can't suggested the arguments. This new method is just a helper to publish the event. The actual logic of updating the state is in setEditCellProps. I can't just call setEditCellProps because the prop won't be called.

dtassone reviewed

View reviewed changes

packages/grid/_modules_/grid/hooks/features/rows/useGridEditRows.ts Outdated Show resolved Hide resolved

dtassone reviewed

View reviewed changes

packages/grid/_modules_/grid/components/cell/GridEditInputCell.tsx Outdated

    
            @@ -25,7 +25,7 @@ export function GridEditInputCell(props: GridCellParams & InputBaseProps) {
          
                    const newValue = event.target.value;

                    const editProps = { value: newValue };

                    setValueState(newValue);

                    api.setEditCellProps({ id, field, props: editProps });

                    api.changeCellEditProps({ id, field, props: editProps }, event);

Member

dtassone Jul 8, 2021

I don't get the purpose of this new method

m4theushw added 2 commits

July 8, 2021 11:31


          typo

022f059


          rename changeCellEditProps to setEditCellValue

eb1d424

dtassone reviewed

View reviewed changes

packages/grid/_modules_/grid/hooks/features/rows/useGridEditRows.ts

-                  const cellCommitParams = apiRef.current.getEditCellPropsParams(params.id, params.field);
-                  if (!cellCommitParams.props.error) {
-                    // We commit the change when there is no error

Member

dtassone Jul 12, 2021

how are you handling this now? error was a sort of predefined prop

Member Author

m4theushw Jul 12, 2021

commitCellChange now checks if there's an error. If there's no error, it publishes GRID_CELL_EDIT_PROPS_CHANGE_COMMITTED. The handler of this event is who updates the row.

dtassone reviewed

View reviewed changes

packages/grid/_modules_/grid/hooks/features/rows/useGridEditRows.ts

@@ @@ -168,6 +163,16 @@ export function useGridEditRows(apiRef: GridApiRef) { @@
                   [apiRef, forceUpdate, logger, setGridState],
                 );
+                const handleCellEditPropsChange = React.useCallback(
+                  (params: GridEditCellPropsParams, event?: React.SyntheticEvent) => {
+                    if (event?.isPropagationStopped()) {

Member

dtassone Jul 12, 2021

Is that not handled in the publishEvent?

Member Author

m4theushw Jul 12, 2021

No, publishEvent prevents from publishing an event if the given event object has the propagation stopped. Once the event is published, it will call all the listeners. If the user wants to stop the propagation of an event in onEditCellChange, we need to check by hand if the propagation was stopped.

Member

dtassone Jul 13, 2021

Yes so one way to do it, is to override the emit behavior of the event emitter so we avoid this check everywhere 🤔

dtassone reviewed

View reviewed changes

packages/grid/_modules_/grid/hooks/features/rows/useGridEditRows.ts

@@ @@ -333,10 +355,11 @@ export function useGridEditRows(apiRef: GridApiRef) { @@
                     getCellMode,
                     isCellEditable,
                     commitCellChange,
-                    setEditCellProps,
+                    setEditCellProps, // TODO don't expose, update the editRowsModel prop directly

Member

dtassone Jul 12, 2021

why this comment?

Member Author

m4theushw Jul 12, 2021

To reduce confusion between setEditCellValue. In edit components, the current value is only thing that needs to be updated in the majority of the cases. Outside an edit components, developers can control the state with the editRowsModel prop to set the error or other prop.


          yarn docs:typescript:formatted

ed6f2f9

Member Author

m4theushw commented Jul 12, 2021

@dtassone We discussed to create another GRID_CELL_EDIT_VALUE_CHANGE event that would be fired by setEditCellValue. I chose to not do it here because GRID_CELL_EDIT_PROPS_CHANGE has only one listener, so it's already this new event. Also, this PR was created just to improve the error validation and to fix a regression. We can go back to it in the useGridControlState's PR.

About the TODO to not expose setEditCellProps, my rationale is that we have two situations where developers might want to update the value and, since they are similar, the name for the methods would cause confusion. The situations are: 1. update the value inside the edit component and 2. update the value/error outside the edit component. For 1. there's setEditCellValue which updates the state and also calls the onEditCellChange prop, so the developer can hook to it a different behavior. In 2. the developer doesn't need the prop to be called, it can directly update the model through the prop. A method for 2. would just be a helper to update the state for a specific id and field.

m4theushw requested review from oliviertassinari and dtassone

July 12, 2021 19:08

dtassone approved these changes

View reviewed changes

m4theushw merged commit a4df486 into mui:master

m4theushw deleted the cell-editing-validation branch

July 13, 2021 19:16

Member

oliviertassinari commented Jul 14, 2021 •

edited

True, GRID_CELL_EDIT_PROPS_CHANGE_COMMITTED shouldn't be fired.

@m4theushw To me it wasn't obvious. In the documentation we use the event to listen to the action that signals that the end-user is done (enter, click away, etc.), we even suggest that validation should be done after. From what I understand we have no documentation for using error: true in the editRowsModel. Do we even need it? How about we remove this feature, waiting for a strong use case to come up?

Member Author

m4theushw commented Jul 15, 2021 •

edited

In the documentation we use the event to listen to the action that signals that the end-user is done (enter, click away, etc.), we even suggest that validation should be done after.

@oliviertassinari That's wrong. Although we say to do the validation after, the demo is doing the validation before, while the user is changing the value. Note that we use the onEditCellChange prop.

From what I understand we have no documentation for using error: true in the editRowsModel. Do we even need it?

Now we have documentation about error: true in #2143. I updated the editing docs to better reflect what we do in the demos: https://deploy-preview-2143--material-ui-x.netlify.app/components/data-grid/editing/#client-side-validation

How about we remove this feature, waiting for a strong use case to come up?

Based on the new docs what do you think we can remove? The GRID_CELL_EDIT_COMMIT (previously named GRID_CELL_EDIT_PROPS_CHANGE_COMMITTED) can be used if the developer wants to send to the server BEFORE updating the row, like to do additional validation in the server.

Member

oliviertassinari commented Jul 17, 2021 •

edited

@m4theushw I didn't saw #2143 when I commented. We are good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment