POC: use react portals for cell editors #1369

amanmahajan7 · 2018-11-04T15:15:13Z

This approach fixes the following problems

Editor is never hidden as it is rendered outside the grid
Currently the editor is using fixed position that requires resetting the editor's position when window is scrolled. This is no longer needed as editor is absolutely positioned.
Additional padding is automatically added when an autocomplete editor is opened on the last row

Fixed a few more issues with frozen columns

Copy/Drag/Selection masks positions
Mask now uses absolute positioning
Removed unused props

This PR however requires a major version as it works with React 16. I think it is about time we drop support for React 15

Demo
https://www.useloom.com/share/45cf203bf19e4027bbb3081dec51d0ad

malonecj · 2018-11-05T11:22:31Z

packages/common/editors/EditorPortal.js

+  el = document.createElement('div');
+
+  componentDidMount() {
+    editorRoot.appendChild(this.el);


I think this makes sense and can fix some of those annoying issues that arise from using fixed positioning

Remove unused props

amanmahajan7

@malonecj I fixed some issues with frozen columns, let me know what you think

amanmahajan7 · 2018-11-07T21:14:12Z

packages/react-data-grid/src/Canvas.js

@@ -218,9 +218,13 @@ class Canvas extends React.PureComponent {
  };

  setScrollLeft = (scrollLeft) => {
+    if (this.interactionMasks && this.interactionMasks.setScrollLeft) {


Using the same approach to fix mask's position

since you are touching this file, maybe we can also add some simple doc above the function? like

/** * what does scrollLeft do * @param scrollLeft number */

amanmahajan7 · 2018-11-07T21:15:13Z

packages/react-data-grid/src/Canvas.js

@@ -246,28 +250,23 @@ class Canvas extends React.PureComponent {
    return this.props.rowHeight * rowIdx;
  }

-  getSelectedRowHeight = (rowIdx) => {


I believe this logic was added to support variable height rows. This adds additional complexity and it is currently not used by Copy and Drag masks so I have removed it. Wdyt?

by saying variable height, do you mean row1 is 50 and row2 is 35 ?

That is correct. This is a great feature to have but we need to handle all the masks (Copy and Drag). This currently does not work with them and has some edge cases so I removed it

I agree that it adds complexity. When starting the work on the Interaction Mask, I did it under the assumption that RDG only supports fixed row height. However, this is not exactly true as it is possible to have dynamic row height using custom row renderers. I have seen a lot of cases out in the wild where this is happening. Removing this, is going to cause a lot of strange display issues for those cases

Supporting dynamic row height is not trivial, and this current implementation was really just a stop gap to support these non documented edge cases. I would prefer to leave this here and think of a better solution that to completely remove it

amanmahajan7 · 2018-11-07T21:15:37Z

packages/react-data-grid/src/Canvas.js

@@ -414,9 +414,6 @@ class Canvas extends React.PureComponent {
          onCellRangeSelectionCompleted={this.props.onCellRangeSelectionCompleted}
          scrollLeft={this._scroll.scrollLeft}
          scrollTop={this._scroll.scrollTop}
-          prevScrollLeft={this.props.prevScrollLeft}


This props do not seem to be used anywhere

I think they can be safely removed.

amanmahajan7 · 2018-11-07T21:16:16Z

packages/react-data-grid/src/masks/InteractionMasks.js

+    }
+  };
+
+  setMaskScollLeft = (mask, position, scrollLeft) => {


This logic adjust the mask position during scroll

amanmahajan7 · 2018-11-07T21:17:28Z

packages/react-data-grid/src/masks/SelectionMask.js

-  return { height, top, width: column.width, left, zIndex };
-};
+import CellMask from './CellMask';
+import { getSelectedDimensions } from '../utils/SelectedCellUtils';


This component can be really simplified if we remove getSelectedRowHeight and getSelectedRowTop

Agreed but I can see some users getting upset that their variable row height grids no longer work. We need to think of a better solution to handle dynamic row heights

qili26

Couple comments.

qili26 · 2018-11-15T15:25:53Z

packages/common/editors/EditorContainer.js

    scrollLeft: PropTypes.number,
-    scrollTop: PropTypes.number
+    scrollTop: PropTypes.number,
+    position: PropTypes.object.isRequired


I am not that familiar with the really old syntax, should we add the shape of the position?

Good idea, I will do that in a separate PR to keep this simple.

qili26 · 2018-11-15T15:30:43Z

packages/common/editors/EditorContainer.js

@@ -132,15 +118,15 @@ class EditorContainer extends React.Component {
      return <CustomEditor ref={this.setEditorRef} {...editorProps} />;
    }

-    return <SimpleTextEditor ref={this.setEditorRef} column={this.props.column} value={this.getInitialValue()} onBlur={this.commit} rowMetaData={this.getRowMetaData()} onKeyDown={() => {}} commit={() => {}}/>;
+    return <SimpleTextEditor ref={this.setEditorRef} column={this.props.column} value={this.getInitialValue()} onBlur={this.commit} rowMetaData={this.getRowMetaData()} onKeyDown={() => { }} commit={() => { }} />;


should we lift the onKeyDown={() => { }} commit={() => { }} to the component like this.emptyCall?

qili26 · 2018-11-15T15:34:23Z

packages/common/editors/EditorContainer.js

-    const { width, height, column } = this.props;
-    const zIndex = isFrozen(column) ? zIndexes.FROZEN_EDITOR_CONTAINER  : zIndexes.EDITOR_CONTAINER;
-    const style = { position: 'fixed', height, width, zIndex, transform: this.calculateTransform() };
+    const { width, height, column, position } = this.props;


This position under this context in render() might be confusing when the dev first look at the code, maybe we can rename it just in this render function? Because position is a valid attr in style, while we are assigning ...position to the style as well. But I am OK if we don't change this.

I have removed position and added left/top props instead

qili26 · 2018-11-15T15:43:13Z

packages/react-data-grid/src/Canvas.js

@@ -218,9 +218,13 @@ class Canvas extends React.PureComponent {
  };

  setScrollLeft = (scrollLeft) => {
+    if (this.interactionMasks && this.interactionMasks.setScrollLeft) {


since you are touching this file, maybe we can also add some simple doc above the function? like

/** * what does scrollLeft do * @param scrollLeft number */

qili26 · 2018-11-15T15:44:11Z

packages/react-data-grid/src/Canvas.js

@@ -246,28 +250,23 @@ class Canvas extends React.PureComponent {
    return this.props.rowHeight * rowIdx;
  }

-  getSelectedRowHeight = (rowIdx) => {


by saying variable height, do you mean row1 is 50 and row2 is 35 ?

qili26 · 2018-11-15T15:56:06Z

packages/react-data-grid/src/masks/InteractionMasks.js

+    if (this.selectionMask) {
+      const { left, top } = this.selectionMask.getBoundingClientRect();
+      const { scrollLeft, scrollTop } = document.documentElement;
+      this.editorPosition = { left: left + scrollLeft, top: top + scrollTop };


I noticed you didn't use this window.pageYOffset in the transform function that I added in the merged PR. Is it because of the this.selectionMask ref? Because the original PR added a window scroll event listener, so that when the grid is at far bottom with window scroll bar and the user drags the window level scroll bar, the mask position will be refreshed. For this InteractionMasks, will the position be refreshed when the window level scroll bar is dragged?

window.pageYOffset is not needed anymore as the editor is now absolutely positioned instead of fixed so it will just work. We however need to make sure scrollleft works for frozen columns and setScrollLeft handles that logic

qili26 · 2018-11-15T22:15:44Z

packages/react-data-grid/src/masks/InteractionMasks.js

+            column={getSelectedColumn({ selectedPosition, columns })}
+            scrollLeft={scrollLeft}
+            scrollTop={scrollTop}
+            {...{


maybe it's better to clean this code like

const restProps = { ...getSelectedDimensions({ selectedPosition, rowHeight, columns, scrollLeft }), ...this.editorPosition }; isEditorEnabled && ( <EditorContainer existing codes {...restProps} /> );

malonecj

Looking good Aman. And thanks for uploading the video. I think this is close to being ready. Some minor comments

malonecj · 2018-11-16T09:22:50Z

packages/common/editors/EditorContainer.js

  };

  onPressEnter = () => {
-    this.commit({key: 'Enter'});
+    this.commit({ key: 'Enter' });


Maybe we can refactor this to use constants instead of magic strings

There is a KeyCodes file that needs to be moved to the common folder. I will address this in a separate PR to keep this one simple

malonecj · 2018-11-16T09:28:35Z

packages/react-data-grid/src/masks/SelectionMask.js

-  return { height, top, width: column.width, left, zIndex };
-};
+import CellMask from './CellMask';
+import { getSelectedDimensions } from '../utils/SelectedCellUtils';


Agreed but I can see some users getting upset that their variable row height grids no longer work. We need to think of a better solution to handle dynamic row heights

malonecj · 2018-11-16T09:29:54Z

packages/react-data-grid/src/masks/InteractionMasks.js

+  setScrollLeft = (scrollLeft) => {
+    const { selectionMask, copyMask, state: { selectedPosition, copiedPosition } } = this;
+    this.setMaskScollLeft(selectionMask, selectedPosition, scrollLeft);
+    this.setMaskScollLeft(copyMask, copiedPosition, scrollLeft);


Does this handle the case of cell range selection as well?

This does not handle cell ranges. Cell range selection probably should only be applied either to the frozen section or to scrollable section. This will require some investigation.

malonecj · 2018-11-16T09:30:36Z

packages/react-data-grid/src/Canvas.js

@@ -414,9 +414,6 @@ class Canvas extends React.PureComponent {
          onCellRangeSelectionCompleted={this.props.onCellRangeSelectionCompleted}
          scrollLeft={this._scroll.scrollLeft}
          scrollTop={this._scroll.scrollTop}
-          prevScrollLeft={this.props.prevScrollLeft}


I think they can be safely removed.

- Add back dynamic row logic

amanmahajan7

I have fixed the logic to calculate dynamic heights for copy and drag masks. There are a few known issues with the current implementation

Select range functionality does not handle dynamic heights. This never worked so I am not fixing it in this PR
Scrolling on cell navigation does not work with dynamic heights. This is due to this logic . I am not sure what is the best fix here.
Placeholder heights are incorrect for dynamic height rows.

I think we should change rowHeight to a function and it will become really simple to support dynamic heights. Let me know what do you think.

amanmahajan7 · 2018-11-19T04:14:08Z

packages/common/editors/EditorContainer.js

  };

  onPressEnter = () => {
-    this.commit({key: 'Enter'});
+    this.commit({ key: 'Enter' });


There is a KeyCodes file that needs to be moved to the common folder. I will address this in a separate PR to keep this one simple

amanmahajan7 · 2018-11-19T04:16:15Z

packages/react-data-grid/src/Canvas.js

    if (wrappedRow) {
      return wrappedRow.row;
    }
    return this.rows[i];
  };

-  getSelectedRowTop = (rowIdx) => {
+  getRowTop = (rowIdx) => {


ReactDOM.findDOMNode() has been deprecated so using an instance method to get row' height/top

amanmahajan7 · 2018-11-19T04:18:08Z

packages/react-data-grid/src/masks/InteractionMasks.js

-      isGroupedRow: this.isGroupedRowSelected(),
-      innerRef: this.setSelectionMaskRef
-    };
+  getSelectedDimensions = (selectedPosition) => {


getSelectedDimensions is used to calculate cell dimensions for all the mask components

amanmahajan7 · 2018-11-19T04:27:59Z

Here is the current implementation

https://www.useloom.com/share/10f73e7d0cd54dcc9c82037fe1706ffb

* POC: use react portals for cell editors * Delete obsolete tests * Fix selection/copy/drag masks positions on frozen columns * Simplify selectionMask position logic Remove unused props * Fix unit tests * Remove getSelectedRowTop * Fix unit tests * Fix unit tests in FF * Remove position prop in favor of left/top props * MInor cleanup * Remove fdescribe and fix unit tests * Remove empty event handlers * Add some comments * - Fix row heights for copy and drag masks - Add back dynamic row logic

POC: use react portals for cell editors

3e75128

amanmahajan7 requested a review from malonecj November 4, 2018 16:11

amanmahajan7 added the WIP label Nov 4, 2018

malonecj approved these changes Nov 5, 2018

View reviewed changes

amanmahajan7 added 4 commits November 7, 2018 08:10

Delete obsolete tests

e9c97ba

Fix selection/copy/drag masks positions on frozen columns

91e2cc2

Simplify selectionMask position logic

6488ef1

Remove unused props

Fix unit tests

61b938d

amanmahajan7 commented Nov 7, 2018

View reviewed changes

amanmahajan7 removed the WIP label Nov 7, 2018

amanmahajan7 added 4 commits November 7, 2018 16:47

Remove getSelectedRowTop

55df83b

Fix unit tests

157aff0

Fix unit tests in FF

111f2c2

Merge branch 'master' into am-editor-portal-poc

0ebd806

amanmahajan7 requested a review from qili26 November 15, 2018 15:21

qili26 reviewed Nov 15, 2018

View reviewed changes

amanmahajan7 added 6 commits November 15, 2018 10:41

Merge branch 'master' into am-editor-portal-poc

81cef8a

Remove position prop in favor of left/top props

89b1143

MInor cleanup

86b7423

Remove fdescribe and fix unit tests

d62f89a

Remove empty event handlers

4b93765

Add some comments

e18bf66

qili26 approved these changes Nov 15, 2018

View reviewed changes

malonecj suggested changes Nov 16, 2018

View reviewed changes

- Fix row heights for copy and drag masks

367b5a8

- Add back dynamic row logic

amanmahajan7 commented Nov 19, 2018

View reviewed changes

malonecj approved these changes Nov 20, 2018

View reviewed changes

amanmahajan7 merged commit 264045f into master Nov 20, 2018

amanmahajan7 deleted the am-editor-portal-poc branch November 20, 2018 17:26

POC: use react portals for cell editors #1369

POC: use react portals for cell editors #1369

Conversation

amanmahajan7 commented Nov 4, 2018 • edited Loading

Choose a reason for hiding this comment

amanmahajan7 left a comment

Choose a reason for hiding this comment

amanmahajan7 Nov 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanmahajan7 Nov 7, 2018 • edited Loading

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

qili26 left a 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

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

malonecj left a 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

amanmahajan7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanmahajan7 Nov 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanmahajan7 commented Nov 19, 2018

amanmahajan7 commented Nov 4, 2018 •

edited

Loading

amanmahajan7 Nov 7, 2018 •

edited

Loading

amanmahajan7 Nov 7, 2018 •

edited

Loading

amanmahajan7 Nov 19, 2018 •

edited

Loading