[DataGrid] Add support for edit components that use portal #1772

m4theushw · 2021-05-26T19:56:24Z

Breaking changes

[DataGrid] The onEditCellChangeCommitted prop won't called with an event when committing changes by clicking outside the cell.

Due to the fact that we relay on the blur event to detect if the user clicked outside the cell, we need to ignore when the focus changes from the root cell element to one of its children. With components that use a portal, this logic doesn't work because there're in a different part of the DOM. This PR changes this behavior to listen for all click events in the document. To filter out events that were inside the cell, we check if before the click a mouseup event was fired. If it does, then the click was inside the cell or inside a portaled element and we shouldn't commit the changes yet. This is the same logic that ClickAwayListener uses. The reason to use mouseup instead of click is to be able to use the Select component.

https://deploy-preview-1772--material-ui-x.netlify.app/components/data-grid/editing/
https://codesandbox.io/s/material-demo-forked-tprys?file=/demo.js

oliviertassinari

It doesn't seem to be working, the edit mode is not excited when clicking another cell: https://deploy-preview-1772--material-ui-x.netlify.app/components/data-grid/editing/#cell-editing

test/e2e/fixtures/DataGrid/PortaledEditCell.tsx

m4theushw · 2021-05-27T01:26:41Z

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 are no error
+      apiRef.current.publishEvent(GRID_CELL_EDIT_PROPS_CHANGE_COMMITTED, cellCommitParams);


I can't pass an event because publishEvent requires a SyntheticEvent (because of the isPropagationStopped) and I only have a native event. That's the reason for the BC label.

Could we make this generic? Otherwise, there would be portal fixes in a lot of features, and each hook will attach and detach events...
If we add an event listener on the doc, and route it to publish some kind of CLICK_OUTSIDE_ROOT
And isolate that into a usePortal hook, it would be really nice.
Then each feature, can hook themselves to some kind of click on portal event...
What do you think?

The click we're interested may occur inside the root as well. What we need to check is if it happened outside the cell that is currently being edited. Since it's a very specific situation, I don't think we need to abstract it. This new event would have to be named like GRID_EDIT_CELL_CLICK_OUTSIDE and it would be attached only to the edit cell, which is the same thing I doing here but without exposing an event.

what if we add a renderCell that use portals? Ie a select component in the grid view mode

what if we add a renderCell that use portals? Ie a select component in the grid view mode

We wouldn't have problems because clicking outside the cell in view mode doesn't do anything besides moving the focus.

unless there is a select dropdown within it

Here's a CodeSandbox with a Select in renderCell: https://codesandbox.io/s/material-demo-forked-kc7id?file=/demo.js

We only have a bug if a portaled component is used in renderEditCell.

m4theushw · 2021-05-27T01:33:56Z

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

@@ -84,13 +101,18 @@ export function useGridEditRows(apiRef: GridApiRef) {
        api: apiRef.current,
      });

+      const doc = ownerDocument(apiRef.current.rootElementRef!.current as HTMLElement);
+      if (mode === 'view') {
+        doc.removeEventListener('click', handleDocumentClick);


I would also attach the listener here when changing to the edit mode but by doing that it broke the regression tests. The click done to switch between tests is incorrectly caught as a click to exit from the edit mode.

m4theushw · 2021-05-27T01:36:31Z

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

-  );
+  const handleCellMouseUp = React.useCallback((params: GridCellParams, event: React.MouseEvent) => {
+    if (event.isPropagationStopped() || params.cellMode === 'edit') {
+      insideCell.current = true;


Here is the magic. If a mouseup came from the cell, then the next click should be treated as it was inside the cell too.

packages/grid/_modules_/grid/hooks/features/keyboard/useGridKeyboard.ts

test/e2e/fixtures/DataGrid/SelectEditCell.tsx

m4theushw · 2021-06-09T02:22:14Z

I changed the way I detect if the click is outside the cell. For that, I created a new GRID_CELL_FOCUS_OUT event which is published by the useGridFocus hook. Different from a blur, it checks if the click came from a portal. @dtassone Could you take a look? I linked in the description a CodeSandbox with a Select or https://deploy-preview-1817--material-ui-x.netlify.app/components/data-grid/demo/.

Troy96 · 2021-06-09T07:46:16Z

@m4theushw Awesome! I have been waiting for this for some time. Can we expect this to go live and be available in coming few days?

dtassone · 2021-06-09T12:10:29Z

packages/grid/_modules_/grid/components/GridRow.tsx

+      // See https://github.com/mui-org/material-ui/issues/10534
+      if (
+        (event.target as any).nodeType === 1 &&
+        !event.currentTarget.contains(event.target as Element)


Could we wrap that into a function such as isTargetWithinElement or isTargetFromSubTree or ?
Make the code more readable

This will trigger the discussion about when to create helpers or not. 😂 I would prefer to keep it as it is because it's only used in two places now. But if we start using more I will definitely create a helper for it.

That logic is pretty complex so I would definitely abstract it.
Normally you can use a rule of 3, here it's repeated twice already, + complexity and readability benefits => clean code

If @oliviertassinari and @DanailH agree we can move this to a helper. I'm against it for the following reasons:

This conditional is only used in two places

We had past discussions about helpers and we didn't reach an agreement

100% of times I see a helper I go to its definition to see what it does, because I don't trust the name

It makes the code cleaner but the developer has to navigate to more files to understand

By the name of the helper I don't know if currentTarget or target is used.

In that particular case, I'm more in favour of keeping it like it is. If you name the helper isTargetWithinElement I would still need to check its implementation to understand what it does. I'm ok with helpers like isSpaceKey for example as it is clear what the helper does.

I'm all 💯 for https://github.com/mui-org/material-ui-x/pull/1772/files#r649207549. Some of the arguments are biased, some are not, but the arguments resonate with me. I also never trust a module I don't know well (nor I think anyone should ever, curiosity is important), having to check the content of small helpers is mentally draining. Two repetitions are not a pattern.

dtassone · 2021-06-09T12:11:01Z

packages/grid/_modules_/grid/components/cell/GridCell.tsx

+      // See https://github.com/mui-org/material-ui/issues/10534
+      if (
+        (event.target as any).nodeType === 1 &&
+        !event.currentTarget.contains(event.target as Element)


reuse it here, remove comments and put it in the helper function

dtassone · 2021-06-09T12:11:57Z

packages/grid/_modules_/grid/hooks/features/keyboard/useGridKeyboard.ts

-      if (!isGridCellRoot(event.target as HTMLElement)) {
+      // The target is not an element when triggered by a Select inside the cell
+      // See https://github.com/mui-org/material-ui/issues/10534
+      if ((event.target as any).nodeType === 1 && !isGridCellRoot(event.target as Element)) {


DanailH · 2021-06-14T11:50:33Z

packages/grid/_modules_/grid/hooks/features/focus/useGridFocus.ts

@@ -28,6 +32,7 @@ export const useGridFocus = (apiRef: GridApiRef): void => {
          focus: { cell: { id, field }, columnHeader: null },
        };
      });
+      // TODO replace with constant
      apiRef.current.publishEvent('cellFocusChange');


Don't we have that event already defined?

There's only a GRID_CELL_FOCUS but it's mapped to the focus event.

DanailH · 2021-06-14T11:52:34Z

packages/grid/_modules_/grid/hooks/features/focus/useGridFocus.ts

+        return;
+      }
+
+      if (params.id === cell.id && params.field === cell.field) {


Can we have a mismatch with the cell if we don't check both the id and the field?

Yes, because this handler is called when any cell is clicked. I just want insideFocusedCell.current to be true when the currently focused cell is clicked.

DanailH · 2021-06-14T11:56:20Z

packages/grid/_modules_/grid/hooks/features/focus/useGridFocus.ts

+      const isInsideFocusedCell = insideFocusedCell.current;
+      insideFocusedCell.current = false;
+
+      const { cell } = apiRef.current.getState().focus;
+      if (!cell || isInsideFocusedCell) {
+        return;
+      }
+


Will this work?

Suggested change

const isInsideFocusedCell = insideFocusedCell.current;

insideFocusedCell.current = false;

const { cell } = apiRef.current.getState().focus;

if (!cell || isInsideFocusedCell) {

return;

}

const { cell } = apiRef.current.getState().focus;

if (!cell || insideFocusedCell.current) {

return;

}

insideFocusedCell.current = false;

There's a problem with setting insideFocusedCell.current to false after checking if there's a cell focused. If there's no cell focused it will stay true, which can prevent the next click from working properly. I borrowed this logic from ClickAwayListener:

https://github.com/mui-org/material-ui/blob/aafb54acb7de52299f4c8cb14f0d4adcaac5cb90/packages/material-ui/src/ClickAwayListener/ClickAwayListener.tsx#L104-L105

oliviertassinari

This demo behaves differently https://deploy-preview-1772--material-ui-x.netlify.app/components/data-grid/editing/#edit-using-external-button. Clicking twice on a cell exit the edit mode

oliviertassinari · 2021-06-16T01:41:11Z

packages/grid/_modules_/grid/components/GridRow.tsx

+      // See https://github.com/mui-org/material-ui/issues/10534
+      if (
+        (event.target as any).nodeType === 1 &&
+        !event.currentTarget.contains(event.target as Element)


I'm all 💯 for https://github.com/mui-org/material-ui-x/pull/1772/files#r649207549. Some of the arguments are biased, some are not, but the arguments resonate with me. I also never trust a module I don't know well (nor I think anyone should ever, curiosity is important), having to check the content of small helpers is mentally draining. Two repetitions are not a pattern.

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

oliviertassinari · 2021-06-16T01:51:25Z

packages/grid/_modules_/grid/hooks/features/focus/useGridFocus.ts

+      }
+
+      if (params.id === cell.id && params.field === cell.field) {
+        insideFocusedCell.current = true;


Did you copy the solution I was proposing for fixing ClickAwayListener with the Select? 😁

oliviertassinari · 2021-06-16T21:35:06Z

@m4theushw On https://deploy-preview-1772--material-ui-x.netlify.app/components/data-grid/editing/#edit-using-external-button. We were previously able to turn an unlimited amount of cells in edit mode. It's no longer the case. The first cell exits as soon as we click another.

packages/grid/x-grid/src/tests/editRows.XGrid.test.tsx

oliviertassinari · 2021-06-16T21:53:09Z

#1772 (comment) Ok, here we go: #1403, point 2. The click event is coming from the document, not managed by React. I guess this resolves the discussion once for all, event.stopPropagation is not flying.

m4theushw · 2021-06-16T22:17:16Z

On https://deploy-preview-1772--material-ui-x.netlify.app/components/data-grid/editing/#edit-using-external-button. We were previously able to turn an unlimited amount of cells in edit mode. It's no longer the case. The first cell exits as soon as we click another.

@oliviertassinari We can't edit multiple cells at the same time anymore because only one cell can have focus.

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

oliviertassinari · 2021-06-16T23:15:23Z

I have fixed the regression on https://deploy-preview-1772--material-ui-x.netlify.app/components/data-grid/editing/#edit-using-external-button and added a regression test case. The fix is relatively simple but there is quite a lot of boilerplate that makes reading the diff a bit more challenging. I guess it's the cost of scaling with an event system, so fair enough.
@m4theushw please review.

⚠️ @dtassone I could only fix the regression with event.defaultMuiPrevented: #1772 (comment).

m4theushw · 2021-06-17T00:07:57Z

@oliviertassinari Looks good to me.

Some points we could bring to #1824:

Now we use event.stopPropagation() and event.defaultMuiPrevented. Maybe it's better to migrate everything to the second option to have only one way to stop propagation.
There's a long list of events to stop propagation to make https://deploy-preview-1772--material-ui-x.netlify.app/components/data-grid/editing/#edit-using-external-button work. We could add a prop to allow users to completely opt-out from the mouse/keyboard interaction.
By allowing to edit multiple cells at the same time, we kinda broke the focus logic. Only one cell could have focus and only the focused cell could be in edit mode.

oliviertassinari · 2021-06-17T00:33:50Z

only the focused cell could be in edit mode.

Why? My assumption was that for row edit, we need to have all the cells of the row in edit mode. It was designed with this constraint in mind so far (I believe).

docs/src/pages/components/data-grid/editing/StartEditButtonGrid.tsx

m4theushw · 2021-06-17T13:11:37Z

packages/grid/_modules_/grid/models/gridOptions.tsx

@@ -31,6 +31,10 @@ import { GridColumnResizeParams } from './params/gridColumnResizeParams';
 import { GridColumnVisibilityChangeParams } from './params/gridColumnVisibilityChangeParams';
 import { GridClasses } from './gridClasses';

+export type MuiEvent<E> = (E | React.SyntheticEvent) & {


@dtassone I added the E type variable so we can reuse it with different native events if necessary.

Nice, but why not do an interface?

Interfaces can't extend union types.

https://stackoverflow.com/questions/56085306/error-message-an-interface-can-only-extend-an-object-type-or-intersection-of-o

because you shouldn't do a union type...
I would have done

interface MuiEvent<E> extends React.SyntheticEvent<E> { ... }

But while typing, I noticed your E | SyntheticEvent. E is any, but yeah it could work anywhere 🤔

Yeah, but this event when triggered by the document is a native event. The union needs to be there.

m4theushw · 2021-06-17T13:23:19Z

Why? My assumption was that for row edit, we need to have all the cells of the row in edit mode. It was designed with this constraint in mind so far (I believe).

@oliviertassinari Probably we'll have problems implementing the row editing, because clicking outside a cell triggers GRID_CELL_FOCUS_OUT but this other cell could be in the same row. The cell editing is tied to the focus logic, which only allows one cell to receive focus. Maybe we need to introduce the row focus.

oliviertassinari · 2021-06-17T14:46:33Z

clicking outside a cell triggers GRID_CELL_FOCUS_OUT but this other cell could be in the same row. The cell editing is tied to the focus logic, which only allows one cell to receive focus. Maybe we need to introduce the row focus.

@m4theushw True, but I wouldn't worry too much about this. It was also the case when relying on the native blur event. In the core, there are a couple of places where we solve this by waiting with a setTimeout() to see if there isn't another focus event, before handling the blur.

Great work!

m4theushw · 2021-06-17T16:24:06Z

@m4theushw Awesome! I have been waiting for this for some time. Can we expect this to go live and be available in coming few days?

@Troy96 Yes, it will be included in the next release which might be this week or next.

m4theushw added bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! labels May 26, 2021

oliviertassinari reviewed May 26, 2021

View reviewed changes

test/e2e/fixtures/DataGrid/PortaledEditCell.tsx Outdated Show resolved Hide resolved

m4theushw added the breaking change label May 27, 2021

m4theushw commented May 27, 2021

View reviewed changes

m4theushw requested review from dtassone and DanailH May 27, 2021 19:34

m4theushw force-pushed the fix-edit-cell-with-portal branch from 810f288 to 1404a43 Compare June 2, 2021 15:17

m4theushw mentioned this pull request Jun 2, 2021

[docs] Make cell editable in demos #1817

Merged

oliviertassinari reviewed Jun 2, 2021

View reviewed changes

packages/grid/_modules_/grid/hooks/features/keyboard/useGridKeyboard.ts Outdated Show resolved Hide resolved

test/e2e/fixtures/DataGrid/SelectEditCell.tsx Outdated Show resolved Hide resolved

m4theushw mentioned this pull request Jun 3, 2021

[DataGrid] Improve API to control cell editing through custom components #1824

Closed

1 task

m4theushw force-pushed the fix-edit-cell-with-portal branch 3 times, most recently from 733b18f to 97a028f Compare June 9, 2021 00:30

dtassone reviewed Jun 9, 2021

View reviewed changes

DanailH reviewed Jun 14, 2021

View reviewed changes

oliviertassinari reviewed Jun 16, 2021

View reviewed changes

m4theushw added 7 commits June 16, 2021 17:07

fix edit cell with portal

8647b72

fix prettier

0e8bd80

save changes if column header is focused

ca19908

fix types

341efef

update focus if cell has no focus

4cc2db8

fix handler name

3a40b7e

typo

97f1ab6

m4theushw force-pushed the fix-edit-cell-with-portal branch from 97370f8 to a6df5e6 Compare June 16, 2021 20:24

m4theushw requested a review from oliviertassinari June 16, 2021 20:37

oliviertassinari mentioned this pull request Jun 16, 2021

[docs] Fix prop-type warning #1916

Merged

oliviertassinari reviewed Jun 16, 2021

View reviewed changes

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

update pattern for the new event

2fae0a7

oliviertassinari mentioned this pull request Jun 16, 2021

[DataGrid] Improve type of the blur event #1918

Merged

yarn prettier && yarn docs:typescript:formatted

1696b9e

m4theushw and others added 3 commits June 16, 2021 19:19

Update packages/grid/x-grid/src/tests/editRows.XGrid.test.tsx

c3efa72

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

Improve event type and forwarding

5528945

add regression test case

a0fd573

oliviertassinari approved these changes Jun 16, 2021

View reviewed changes

yarn docs:api

1dfb3f3

DanailH approved these changes Jun 17, 2021

View reviewed changes

dtassone reviewed Jun 17, 2021

View reviewed changes

docs/src/pages/components/data-grid/editing/StartEditButtonGrid.tsx Outdated Show resolved Hide resolved

m4theushw added 2 commits June 17, 2021 09:57

add MuiEvent

47f169a

Merge branch 'master' into fix-edit-cell-with-portal

99cfeb0

m4theushw commented Jun 17, 2021

View reviewed changes

m4theushw requested a review from dtassone June 17, 2021 13:11

m4theushw merged commit 9309c2e into mui:master Jun 17, 2021

m4theushw deleted the fix-edit-cell-with-portal branch June 17, 2021 16:24

m4theushw mentioned this pull request Jul 19, 2021

[DataGrid] Double-clicking a portaled cell doesn't trigger the edit mode #2170

Closed

2 tasks

[DataGrid] Add support for edit components that use portal #1772

[DataGrid] Add support for edit components that use portal #1772

Conversation

m4theushw commented May 26, 2021 • edited

Breaking changes

oliviertassinari left a comment

Choose a reason for hiding this comment

m4theushw May 27, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m4theushw commented Jun 9, 2021 • edited

Troy96 commented Jun 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m4theushw Jun 10, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Jun 16, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m4theushw Jun 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari left a comment

Choose a reason for hiding this comment

oliviertassinari Jun 16, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented Jun 16, 2021

oliviertassinari commented Jun 16, 2021 • edited

m4theushw commented Jun 16, 2021

oliviertassinari commented Jun 16, 2021 • edited

m4theushw commented Jun 17, 2021

oliviertassinari commented Jun 17, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m4theushw commented Jun 17, 2021

oliviertassinari commented Jun 17, 2021 • edited

m4theushw commented Jun 17, 2021

m4theushw commented May 26, 2021 •

edited

m4theushw May 27, 2021 •

edited

m4theushw commented Jun 9, 2021 •

edited

m4theushw Jun 10, 2021 •

edited

oliviertassinari Jun 16, 2021 •

edited

m4theushw Jun 14, 2021 •

edited

oliviertassinari Jun 16, 2021 •

edited

oliviertassinari commented Jun 16, 2021 •

edited

oliviertassinari commented Jun 16, 2021 •

edited

oliviertassinari commented Jun 17, 2021 •

edited

oliviertassinari commented Jun 17, 2021 •

edited