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

Context menu upgrade from v1.6.2 to v2.9.2 #1081

Merged
merged 8 commits into from
Feb 8, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
"fs-extra": "^0.30.0",
"object-assign": "^2.0.0",
"prop-types": "^15.5.10",
"react-contextmenu": "1.6.2",
"react-contextmenu": "^2.9.2",
"react-dnd": "^2.1.4",
"react-dnd-html5-backend": "^2.1.2",
"react-input-autosize": "1.1.0",
Expand Down
5 changes: 2 additions & 3 deletions packages/react-data-grid-addons/src/menu/index.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import {MenuItem, monitor, SubMenu, connect, ContextMenuLayer} from 'react-contextmenu';
import ContextMenu from './ContextMenu';
import {ContextMenu, MenuItem, monitor, SubMenu, connect, ContextMenuTrigger} from 'react-contextmenu';
import MenuHeader from './MenuHeader';

export {ContextMenu, MenuHeader, MenuItem, monitor, SubMenu, connect, ContextMenuLayer};
export {ContextMenu, MenuHeader, MenuItem, monitor, SubMenu, connect, ContextMenuTrigger};
Original file line number Diff line number Diff line change
@@ -1,47 +1,78 @@
.react-context-menu {
.react-contextmenu {
min-width: 160px;
padding: 5px 0;
margin: 2px 0 0;
font-size: 14px;
font-size: 16px;
color: #373a3c;
text-align: left;
background-color: #fff;
-webkit-background-clip: padding-box;
background-clip: padding-box;
border: 1px solid rgba(0,0,0,.15);
border-radius: .25rem;
outline: none;
opacity: 0;
pointer-events: none;
transition: opacity 250ms ease !important;
}

.react-context-menu-link {
display: inline-block;
width: 100%;
.react-contextmenu.react-contextmenu--visible {
opacity: 1;
pointer-events: auto;
}

.react-contextmenu-item {
padding: 3px 20px;
clear: both;
font-weight: 400;
line-height: 1.5;
color: #373a3c;
text-align: inherit;
white-space: nowrap;
background: 0 0;
border: 0;
cursor: pointer;
}

.react-context-menu-link.active,
.react-context-menu-link:hover {
.react-contextmenu-item.react-contextmenu-item--active,
.react-contextmenu-item.react-contextmenu-item--selected {
color: #fff;
background-color: #0275d8;
border-color: #0275d8;
background-color: #20a0ff;
border-color: #20a0ff;
text-decoration: none;
}

.react-context-menu-item.submenu > a {
padding-right: 27px;
.react-contextmenu-item.react-contextmenu-item--disabled,
.react-contextmenu-item.react-contextmenu-item--disabled:hover {
color: #878a8c;
background-color: transparent;
border-color: rgba(0,0,0,.15);
}

.react-contextmenu-item--divider {
margin-bottom: 3px;
padding: 2px 0;
border-bottom: 1px solid rgba(0,0,0,.15);
cursor: inherit;
}
.react-contextmenu-item--divider:hover {
background-color: transparent;
border-color: rgba(0,0,0,.15);
}

.react-context-menu-item.submenu > a:after {
.react-contextmenu-item.react-contextmenu-submenu {
padding: 0;
}

.react-contextmenu-item.react-contextmenu-submenu > .react-contextmenu-item {
}

.react-contextmenu-item.react-contextmenu-submenu > .react-contextmenu-item:after {
content: "▶";
display: inline-block;
position: absolute;
right: 7px;
}

.example-multiple-targets::after {
content: attr(data-count);
display: block;
}
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ class Example extends React.Component {
return (
<ReactDataGrid
contextMenu={<MyContextMenu onRowDelete={this.deleteRow} onRowInsertAbove={this.insertRowAbove} onRowInsertBelow={this.insertRowBelow} />}
contextMenuId="myContextMenu"
columns={this._columns}
rowGetter={this.rowGetter}
rowsCount={this.state.rows.length}
Expand All @@ -76,7 +77,8 @@ class MyContextMenu extends React.Component {
onRowInsertAbove: PropTypes.func.isRequired,
onRowInsertBelow: PropTypes.func.isRequired,
rowIdx: PropTypes.string.isRequired,
idx: PropTypes.string.isRequired
idx: PropTypes.string.isRequired,
id: PropTypes.string.isRequired
};

onRowDelete = (e, data) => {
Expand All @@ -98,12 +100,14 @@ class MyContextMenu extends React.Component {
};

render() {
const { idx, id, rowIdx } = this.props;

return (
<ContextMenu>
<MenuItem data={{rowIdx: this.props.rowIdx, idx: this.props.idx}} onClick={this.onRowDelete}>Delete Row</MenuItem>
<ContextMenu id={id}>
<MenuItem data={{ rowIdx, idx }} onClick={this.onRowDelete}>Delete Row</MenuItem>
<SubMenu title="Insert Row">
<MenuItem data={{rowIdx: this.props.rowIdx, idx: this.props.idx}} onClick={this.onRowInsertAbove}>Above</MenuItem>
<MenuItem data={{rowIdx: this.props.rowIdx, idx: this.props.idx}} onClick={this.onRowInsertBelow}>Below</MenuItem>
<MenuItem data={{ rowIdx, idx }} onClick={this.onRowInsertAbove}>Above</MenuItem>
<MenuItem data={{ rowIdx, idx }} onClick={this.onRowInsertBelow}>Below</MenuItem>
</SubMenu>
</ContextMenu>
);
Expand Down
2 changes: 2 additions & 0 deletions packages/react-data-grid/src/Canvas.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ const Canvas = createReactClass({
rowScrollTimeout: PropTypes.number,
scrollToRowIndex: PropTypes.number,
contextMenu: PropTypes.element,
contextMenuId: PropTypes.string,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does grid need to specify contextMenuId prop. id can be specified while creating contextMenu element so the context menu example can be written as

<ReactDataGrid
        contextMenu={<MyContextMenu 
        id="myContextMenu" // ContextMenu ID
        onRowDelete={this.deleteRow} 
        onRowInsertAbove={this.insertRowAbove} 
        onRowInsertBelow={this.insertRowBelow} />}

....

This way contextMenuId prop does not need to pass down from ReactDataGrid to RowsContainer

Thoughts?

getSubRowDetails: PropTypes.func,
rowSelection: PropTypes.oneOfType([
PropTypes.shape({
Expand Down Expand Up @@ -333,6 +334,7 @@ const Canvas = createReactClass({
width={this.props.width}
rows={rows}
contextMenu={this.props.contextMenu}
contextMenuId={this.props.contextMenuId}
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the above comment contextMenuId prop can be removed from all the components

rowIdx={this.props.cellMetaData.selected.rowIdx}
idx={this.props.cellMetaData.selected.idx} />
</div>
Expand Down
2 changes: 2 additions & 0 deletions packages/react-data-grid/src/Grid.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ const Grid = createReactClass({
rowScrollTimeout: PropTypes.number,
scrollToRowIndex: PropTypes.number,
contextMenu: PropTypes.element,
contextMenuId: React.PropTypes.string,
getSubRowDetails: PropTypes.func,
draggableHeaderCell: PropTypes.func,
getValidFilterValues: PropTypes.func,
Expand Down Expand Up @@ -138,6 +139,7 @@ const Grid = createReactClass({
rowScrollTimeout={this.props.rowScrollTimeout}
scrollToRowIndex={this.props.scrollToRowIndex}
contextMenu={this.props.contextMenu}
contextMenuId={this.props.contextMenuId}
rowSelection={this.props.rowSelection}
getSubRowDetails={this.props.getSubRowDetails}
rowGroupRenderer={this.props.rowGroupRenderer}
Expand Down
2 changes: 2 additions & 0 deletions packages/react-data-grid/src/ReactDataGrid.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ const ReactDataGrid = createReactClass({
scrollToRowIndex: PropTypes.number,
onClearFilters: PropTypes.func,
contextMenu: PropTypes.element,
contextMenuId: React.PropTypes.string,
cellNavigationMode: PropTypes.oneOf(['none', 'loopOverRow', 'changeRow']),
onCellSelected: PropTypes.func,
onCellDeSelected: PropTypes.func,
Expand Down Expand Up @@ -1103,6 +1104,7 @@ const ReactDataGrid = createReactClass({
rowScrollTimeout={this.props.rowScrollTimeout}
scrollToRowIndex={this.props.scrollToRowIndex}
contextMenu={this.props.contextMenu}
contextMenuId={this.props.contextMenuId}
overScan={this.props.overScan} />
</div>
</div>
Expand Down
44 changes: 25 additions & 19 deletions packages/react-data-grid/src/RowsContainer.js
Original file line number Diff line number Diff line change
@@ -1,35 +1,33 @@
import React from 'react';
import PropTypes from 'prop-types';

const SimpleRowsContainer = (props) => {
return (
<div key="rows-container">
{props.rows}
</div>
);
};
export const DEFAULT_CONTEXT_MENU_ID = 'rgdContextMenu';

const SimpleRowsContainer = (props) => <div key="rows-container">{props.rows}</div>;

SimpleRowsContainer.propTypes = {
width: PropTypes.number,
rows: PropTypes.array
};

export const getNewContextMenuProps = ({ contextMenuId, rowIdx, idx }) => ({
rowIdx, idx, id: contextMenuId || DEFAULT_CONTEXT_MENU_ID
});

class RowsContainer extends React.Component {
constructor(props) {
super(props);
this.plugins = props.window ? props.window.ReactDataGridPlugins : window.ReactDataGridPlugins;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an old code but is there a use case of props.window? I wonder if anyone is actually using it, I do not think it can even be populated. This can be cleaned up (probably in a separate PR)

https://github.com/adazzle/react-data-grid/blob/master/packages/react-data-grid/src/Canvas.js#L284

Copy link
Contributor

Choose a reason for hiding this comment

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

@amanmahajan7 can it be server side rendering?
I think this was added back in the days due to IE8 support problems, having the context menu added directly would break IE8 apart

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 sure but that does seem like a workaround for SSR. We can revisit this code once RDG officially supports server side rendering.

this.hasContextMenu = this.hasContextMenu.bind(this);
this.renderRowsWithContextMenu = this.renderRowsWithContextMenu.bind(this);
this.getContextMenuContainer = this.getContextMenuContainer.bind(this);
this.state = {ContextMenuContainer: this.getContextMenuContainer(props)};
this.validateContextMenu = this.validateContextMenu.bind(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

These methods are not used as event handlers and do not need to be bound. They are used in the constructor and in the render method where this value would be correct


this.validateContextMenu();
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if constructor is the right place for validation. Did not find any recommendation on where to throw the errors but I think render method would probably be a better place. It is recommended to avoid any side effects in constructor
https://reactjs.org/docs/react-component.html#constructor

render() {
    if (this.hasContextMenu())  {
        if (!this.plugins) { throw new Error('...') }
        return this.renderRowsWithContextMenu();
    }
    return <SimpleRowsContainer {...this.props} />;
}

@malonecj @diogofcunha thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

@amanmahajan7 we shouldn't add any side effects in the constructor even when not using react, creating a new object should never affect other parts of an application, you are correct, in react the constructor is used only to set the initial state

}

getContextMenuContainer() {
if (this.hasContextMenu()) {
if (!this.plugins) {
throw new Error('You need to include ReactDataGrid UiPlugins in order to initialise context menu');
}
return this.plugins.Menu.ContextMenuLayer('reactDataGridContextMenu')(SimpleRowsContainer);
validateContextMenu() {
if (this.hasContextMenu() && !this.plugins) {
throw new Error('You need to include ReactDataGrid UiPlugins in order to initialise context menu');
}
}

Expand All @@ -38,11 +36,18 @@ class RowsContainer extends React.Component {
}

renderRowsWithContextMenu() {
let ContextMenuRowsContainer = this.state.ContextMenuContainer;
let newProps = {rowIdx: this.props.rowIdx, idx: this.props.idx};
let contextMenu = React.cloneElement(this.props.contextMenu, newProps);
const { ContextMenuTrigger } = this.plugins.Menu;
const newProps = getNewContextMenuProps(this.props);
const contextMenu = React.cloneElement(this.props.contextMenu, newProps);
// Initialise the context menu if it is available
return (<div><ContextMenuRowsContainer {...this.props} />{contextMenu}</div>);
return (
<div>
<ContextMenuTrigger id={newProps.id}>
<SimpleRowsContainer {...this.props} />
</ContextMenuTrigger>
{contextMenu}
</div>
);
}

render() {
Expand All @@ -52,6 +57,7 @@ class RowsContainer extends React.Component {

RowsContainer.propTypes = {
contextMenu: PropTypes.element,
contextMenuId: PropTypes.string,
rowIdx: PropTypes.number,
idx: PropTypes.number,
window: PropTypes.object
Expand Down
2 changes: 2 additions & 0 deletions packages/react-data-grid/src/Viewport.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ const Viewport = createReactClass({
rowScrollTimeout: PropTypes.number,
scrollToRowIndex: PropTypes.number,
contextMenu: PropTypes.element,
contextMenuId: PropTypes.string,
getSubRowDetails: PropTypes.func,
rowGroupRenderer: PropTypes.func
},
Expand Down Expand Up @@ -107,6 +108,7 @@ const Viewport = createReactClass({
rowScrollTimeout={this.props.rowScrollTimeout}
scrollToRowIndex={this.props.scrollToRowIndex}
contextMenu={this.props.contextMenu}
contextMenuId={this.props.contextMenuId}
rowSelection={this.props.rowSelection}
getSubRowDetails={this.props.getSubRowDetails}
rowGroupRenderer={this.props.rowGroupRenderer}
Expand Down
64 changes: 57 additions & 7 deletions packages/react-data-grid/src/__tests__/RowsContainer.spec.js
Original file line number Diff line number Diff line change
@@ -1,17 +1,67 @@
import { shallow } from 'enzyme';
import React from 'react';
import ReactTestUtils from 'react-addons-test-utils';
import RowsContainer from '../RowsContainer';
import RowsContainer, { getNewContextMenuProps, SimpleRowsContainer, DEFAULT_CONTEXT_MENU_ID } from '../RowsContainer';

const FakeContextMenuTrigger = () => <div id="fakeContextMenuTrigger" />;

const FakeContextMenu = () => <div id="fakeContextMenu" />;

const ReactDataGridPlugins = {
Menu: {
ContextMenuTrigger: FakeContextMenuTrigger
}
};

const props = {
contextMenu: <FakeContextMenu />,
contextMenuId: 'fakeContextMenu',
rowIdx: 5,
idx: 8,
window: { ReactDataGridPlugins },
rows: [
{ id: 'row_1' },
{ id: 'row_2' }
]
};

describe('Rows Container', () => {
describe('without context menu', () => {
let componentWithoutContextMenu = {};
describe('getNewContextMenuProps()', () => {
it('should populate correct newProps for contextMenu with customized menu id', () => {
const newProps = getNewContextMenuProps(props);
expect(newProps.id).toBe(props.contextMenuId);
});

beforeEach(() => {
componentWithoutContextMenu = ReactTestUtils.renderIntoDocument(<RowsContainer window={{ ReactDataGridPlugins: undefined }} />);
it('should populate correct newProps for contextMenu with default menu id', () => {
const newProps = getNewContextMenuProps(Object.assign({}, props, { contextMenuId: undefined }));
expect(newProps.id).toBe(DEFAULT_CONTEXT_MENU_ID);
});
});

describe('with context menu', () => {
it('should create a new RowsContainer instance', () => {
expect(componentWithoutContextMenu).toBeDefined();
const wrapper = shallow(<RowsContainer {...props} />);
expect(wrapper.find(FakeContextMenuTrigger).length).toBe(1);
});

it('should throw exception for no context menu plugin', () => {
const newProp = Object.assign({}, props, { window: {}});
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

toThrow/toThrowError can be used instead

expect(shallow(<RowsContainer {...newProp} />)).toThrow()

shallow(<RowsContainer {...newProp} />);
} catch (e) {
expect(e.message).toContain('You need to include ReactDataGrid UiPlugins in order to initialise context menu');
}
});
});

describe('without context menu', () => {
it('should create a SimpleRowsContainer', () => {
const newProps = Object.assign({}, props, {
contextMenu: undefined,
contextMenuId: undefined
});
const wrapper = shallow(<RowsContainer {...newProps}/>);
expect(wrapper.find(SimpleRowsContainer).length).toBe(1);
expect(wrapper.find(FakeContextMenuTrigger).length).toBe(0);
});
});
});