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.

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

POC: use react portals for cell editors #1369

Merged
merged 16 commits into from
Nov 20, 2018
Merged

POC: use react portals for cell editors #1369

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
3e75128
POC: use react portals for cell editors
amanmahajan7 Nov 4, 2018
e9c97ba
Delete obsolete tests
amanmahajan7 Nov 7, 2018
91e2cc2
Fix selection/copy/drag masks positions on frozen columns
amanmahajan7 Nov 7, 2018
6488ef1
Simplify selectionMask position logic
amanmahajan7 Nov 7, 2018
61b938d
Fix unit tests
amanmahajan7 Nov 7, 2018
55df83b
Remove getSelectedRowTop
amanmahajan7 Nov 7, 2018
157aff0
Fix unit tests
amanmahajan7 Nov 8, 2018
111f2c2
Fix unit tests in FF
amanmahajan7 Nov 8, 2018
0ebd806
Merge branch 'master' into am-editor-portal-poc
amanmahajan7 Nov 14, 2018
81cef8a
Merge branch 'master' into am-editor-portal-poc
amanmahajan7 Nov 15, 2018
89b1143
Remove position prop in favor of left/top props
amanmahajan7 Nov 15, 2018
86b7423
MInor cleanup
amanmahajan7 Nov 15, 2018
d62f89a
Remove fdescribe and fix unit tests
amanmahajan7 Nov 15, 2018
4b93765
Remove empty event handlers
amanmahajan7 Nov 15, 2018
e18bf66
Add some comments
amanmahajan7 Nov 15, 2018
367b5a8
- Fix row heights for copy and drag masks
amanmahajan7 Nov 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 29 additions & 36 deletions packages/common/editors/EditorContainer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@ import React from 'react';
import PropTypes from 'prop-types';
import joinClasses from 'classnames';
import SimpleTextEditor from './SimpleTextEditor';
import {isFunction} from 'common/utils';
import { isFunction } from 'common/utils';
import { isKeyPrintable, isCtrlKeyHeldDown } from 'common/utils/keyboardUtils';
import zIndexes from 'common/constants/zIndexes';
import EditorPortal from './EditorPortal';

require('../../../themes/react-data-grid-core.css');

const isFrozen = column => column.frozen === true || column.locked === true;
Expand All @@ -22,14 +24,13 @@ class EditorContainer extends React.Component {
onCommit: PropTypes.func,
onCommitCancel: PropTypes.func,
firstEditorKeyPress: PropTypes.string,
width: PropTypes.number,
top: PropTypes.number,
left: PropTypes.number,
width: PropTypes.number.isRequired,
scrollLeft: PropTypes.number,
scrollTop: PropTypes.number
scrollTop: PropTypes.number,
position: PropTypes.object.isRequired
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

};

state = {isInvalid: false};
state = { isInvalid: false };
changeCommitted = false;
changeCanceled = false;

Expand All @@ -42,7 +43,6 @@ class EditorContainer extends React.Component {
inputNode.style.height = this.props.height - 1 + 'px';
}
}
window.addEventListener('scroll', this.setContainerPosition);
}

componentDidUpdate(prevProps) {
Expand All @@ -53,24 +53,10 @@ class EditorContainer extends React.Component {

componentWillUnmount() {
if (!this.changeCommitted && !this.changeCanceled) {
this.commit({key: 'Enter'});
}
window.removeEventListener('scroll', this.setContainerPosition);
}

setContainerPosition = () => {
if (this.container) {
this.container.style.transform = this.calculateTransform();
this.commit({ key: 'Enter' });
}
}

calculateTransform = () => {
const { column, left, scrollLeft, top, scrollTop } = this.props;
const editorLeft = isFrozen(column) ? left : left - scrollLeft;
const editorTop = top - scrollTop - window.pageYOffset;
return `translate(${editorLeft}px, ${editorTop}px)`;
}

isKeyExplicitlyHandled = (key) => {
return isFunction(this['onPress' + key]);
};
Expand Down Expand Up @@ -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={() => { }} />;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

};

onPressEnter = () => {
this.commit({key: 'Enter'});
this.commit({ key: 'Enter' });
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

};

onPressTab = () => {
this.commit({key: 'Tab'});
this.commit({ key: 'Tab' });
};

onPressEscape = (e) => {
Expand Down Expand Up @@ -239,13 +225,13 @@ class EditorContainer extends React.Component {
};

commit = (args) => {
const {onCommit} = this.props;
const { onCommit } = this.props;
let opts = args || {};
let updated = this.getEditor().getValue();
if (this.isNewValueValid(updated)) {
this.changeCommitted = true;
let cellKey = this.props.column.key;
onCommit({cellKey: cellKey, rowIdx: this.props.rowIdx, updated: updated, key: opts.key});
onCommit({ cellKey: cellKey, rowIdx: this.props.rowIdx, updated: updated, key: opts.key });
}
};

Expand All @@ -257,7 +243,7 @@ class EditorContainer extends React.Component {
isNewValueValid = (value) => {
if (isFunction(this.getEditor().validate)) {
let isValid = this.getEditor().validate(value);
this.setState({isInvalid: !isValid});
this.setState({ isInvalid: !isValid });
return isValid;
}

Expand Down Expand Up @@ -306,8 +292,8 @@ class EditorContainer extends React.Component {

getRelatedTarget = (e) => {
return e.relatedTarget ||
e.explicitOriginalTarget ||
document.activeElement; // IE11
e.explicitOriginalTarget ||
document.activeElement; // IE11
};

handleRightClick = (e) => {
Expand All @@ -321,7 +307,7 @@ class EditorContainer extends React.Component {
}

if (!this.isBodyClicked(e)) {
// prevent null reference
// prevent null reference
if (this.isViewportClicked(e) || !this.isClickInsideEditor(e)) {
this.commit(e);
}
Expand Down Expand Up @@ -349,15 +335,22 @@ class EditorContainer extends React.Component {
};

render() {
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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have removed position and added left/top props instead

const zIndex = isFrozen(column) ? zIndexes.FROZEN_EDITOR_CONTAINER : zIndexes.EDITOR_CONTAINER;
const style = { position: 'absolute', height, width, zIndex, ...position };
return (
<div ref={this.setContainerRef} style={style} className={this.getContainerClass()} onBlur={this.handleBlur} onKeyDown={this.onKeyDown} onContextMenu={this.handleRightClick}>
<EditorPortal>
<div style={style}
className={this.getContainerClass()}
onBlur={this.handleBlur}
onKeyDown={this.onKeyDown}
onContextMenu={this.handleRightClick}
>
{this.createEditor()}
{this.renderStatusIcon()}
</div>
);
</EditorPortal>
);
}
}

Expand Down
28 changes: 28 additions & 0 deletions packages/common/editors/EditorPortal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import React from 'react';
import ReactDOM from 'react-dom';
import PropTypes from 'prop-types';

const editorRoot = document.body;

export default class EditorPortal extends React.Component {
static propTypes = {
children: PropTypes.node
};

el = document.createElement('div');

componentDidMount() {
editorRoot.appendChild(this.el);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

componentWillUnmount() {
editorRoot.removeChild(this.el);
}

render() {
return ReactDOM.createPortal(
this.props.children,
this.el,
);
}
}
37 changes: 7 additions & 30 deletions packages/common/editors/__tests__/EditorContainer.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ const defaultProps = {
},
column: fakeColumn,
value: 'Adwolf',
height: 50
height: 50,
onCommit: jasmine.createSpy(),
onCommitCancel: jasmine.createSpy()
};

const getComponent = (props) => {
Expand Down Expand Up @@ -77,31 +79,6 @@ describe('Editor Container Tests', () => {
expect(editorDiv.props().onKeyDown).toBeDefined();
expect(editorDiv.props().children).toBeDefined();
});

describe('Frozen columns', () => {
const frozenProps = {
column: { ...fakeColumn, frozen: true },
left: 60,
top: 0,
scrollTop: 0,
scrollLeft: 250
};

it('should not subtract scrollLeft value from editors left position when column is frozen', () => {
const { shallowWrapper } = getComponent(frozenProps);
const editorDiv = shallowWrapper.find('div').at(0);
expect(editorDiv.props().style.transform).toBe('translate(60px, 0px)');
});

it('should subtract scrollLeft value from editors left position when column is not frozen', () => {
const unfrozenProps = { ...frozenProps };
unfrozenProps.column.frozen = false;

const { shallowWrapper } = getComponent(unfrozenProps);
const editorDiv = shallowWrapper.find('div').at(0);
expect(editorDiv.props().style.transform).toBe('translate(-190px, 0px)');
});
});
});

describe('Custom Editors', () => {
Expand Down Expand Up @@ -190,12 +167,11 @@ describe('Editor Container Tests', () => {
let container;

beforeEach(() => {
defaultProps.onCommit.calls.reset();
defaultProps.onCommitCancel.calls.reset();
container = document.createElement('div');
document.body.appendChild(container);
component = mount(<EditorContainer
onCommit={jasmine.createSpy()}
onCommitCancel={jasmine.createSpy()}
{ ...defaultProps }/>, { attachTo: container });
component = mount(<EditorContainer { ...defaultProps }/>, { attachTo: container });
});

afterEach(() => {
Expand All @@ -215,6 +191,7 @@ describe('Editor Container Tests', () => {
const editor = component.find(SimpleTextEditor);

editor.simulate('keydown', { key: 'Escape' });
component.detach();

expect(component.props().onCommitCancel).toHaveBeenCalled();
expect(component.props().onCommitCancel.calls.count()).toEqual(1);
Expand Down
4 changes: 2 additions & 2 deletions packages/react-data-grid-addons/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
"react-data-grid": "^5.0.4"
},
"peerDependencies": {
"react": "^15.0.0 || ^16.0.0",
"react-dom": "^15.0.0 || ^16.0.0"
"react": "^16.0.0",
"react-dom": "^16.0.0"
}
}
4 changes: 2 additions & 2 deletions packages/react-data-grid/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
"author": "Adazzle",
"license": "MIT",
"peerDependencies": {
"react": "^15.0.0 || ^16.0.0",
"react-dom": "^15.0.0 || ^16.0.0"
"react": "^16.0.0",
"react-dom": "^16.0.0"
}
}
39 changes: 13 additions & 26 deletions packages/react-data-grid/src/Canvas.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import React from 'react';
import ReactDOM from 'react-dom';
import PropTypes from 'prop-types';

import Row from './Row';
import cellMetaDataShape from 'common/prop-shapes/CellActionShape';
import * as rowUtils from './RowUtils';
Expand Down Expand Up @@ -218,9 +218,13 @@ class Canvas extends React.PureComponent {
};

setScrollLeft = (scrollLeft) => {
if (this.interactionMasks && this.interactionMasks.setScrollLeft) {
Copy link
Contributor Author

@amanmahajan7 amanmahajan7 Nov 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Using the same approach to fix mask's position

Copy link
Contributor

Choose a reason for hiding this comment

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

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
 */

this.interactionMasks.setScrollLeft(scrollLeft);
}

this.rows.forEach((r, idx) => {
if (r) {
let row = this.getRowByRef(idx);
const row = this.getRowByRef(idx);
if (row && row.setScrollLeft) {
row.setScrollLeft(scrollLeft);
}
Expand All @@ -237,37 +241,23 @@ class Canvas extends React.PureComponent {
return this.rows[i];
};

getSelectedRowTop = (rowIdx) => {
const row = this.getRowByRef(rowIdx);
if (row) {
const node = ReactDOM.findDOMNode(row);
return node && node.offsetTop;
}
return this.props.rowHeight * rowIdx;
}

getSelectedRowHeight = (rowIdx) => {
Copy link
Contributor Author

@amanmahajan7 amanmahajan7 Nov 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

const row = this.getRowByRef(rowIdx);
if (row) {
const node = ReactDOM.findDOMNode(row);
return node && node.clientHeight > 0 ? node.clientHeight : this.props.rowHeight;
}
return this.props.rowHeight;
}

getSelectedRowColumns = (rowIdx) => {
const row = this.getRowByRef(rowIdx);
return row && row.props ? row.props.columns : this.props.columns;
}

setCanvasRef = (canvas) => {
this.canvas = canvas;
setCanvasRef = el => {
this.canvas = el;
};

setRowRef = idx => row => {
this.rows[idx] = row;
};

setInteractionMasksRef = el => {
this.interactionMasks = el;
};

renderCustomRowRenderer(props) {
const { ref, ...otherProps } = props;
const CustomRowRenderer = this.props.rowRenderer;
Expand Down Expand Up @@ -382,6 +372,7 @@ class Canvas extends React.PureComponent {
onScroll={this.onScroll}
className="react-grid-Canvas">
<InteractionMasks
ref={this.setInteractionMasksRef}
rowGetter={rowGetter}
rowsCount={rowsCount}
width={this.props.totalWidth}
Expand Down Expand Up @@ -414,10 +405,6 @@ class Canvas extends React.PureComponent {
onCellRangeSelectionCompleted={this.props.onCellRangeSelectionCompleted}
scrollLeft={this._scroll.scrollLeft}
scrollTop={this._scroll.scrollTop}
prevScrollLeft={this.props.prevScrollLeft}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This props do not seem to be used anywhere

Copy link
Contributor

Choose a reason for hiding this comment

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

I think they can be safely removed.

prevScrollTop={this.props.prevScrollTop}
getSelectedRowHeight={this.getSelectedRowHeight}
getSelectedRowTop={this.getSelectedRowTop}
getSelectedRowColumns={this.getSelectedRowColumns}
/>
<RowsContainer id={contextMenu ? contextMenu.props.id : 'rowsContainer'}>
Expand Down
4 changes: 1 addition & 3 deletions packages/react-data-grid/src/Viewport.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,9 +137,7 @@ class Viewport extends React.Component {
colOverscanEndIdx,
scrollDirection,
lastFrozenColumnIndex,
isScrolling,
prevScrollTop: this.state.scrollTop,
prevScrollLeft: this.state.scrollTop
isScrolling
};
}

Expand Down
4 changes: 0 additions & 4 deletions packages/react-data-grid/src/__tests__/Viewport.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,6 @@ describe('<Viewport />', () => {
height: viewportProps.minHeight,
scrollLeft: scrollLeft,
scrollTop: scrollTop,
prevScrollTop: 0,
prevScrollLeft: 0,
rowVisibleEndIdx: 21,
rowVisibleStartIdx: 6,
isScrolling: true,
Expand Down Expand Up @@ -153,8 +151,6 @@ describe('<Viewport />', () => {
height: newHeight,
scrollLeft: 0,
scrollTop: 0,
prevScrollTop: 0,
prevScrollLeft: 0,
rowVisibleEndIdx: 29,
rowVisibleStartIdx: 0,
isScrolling: true,
Expand Down
Loading