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

Conversation

Projects
None yet
6 participants
@qili26
Copy link
Contributor

qili26 commented Jan 15, 2018

Description

A few sentences describing the overall goals of the pull request's commits.

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x")

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)
react data grid + react v16 + current react context menu v1.6.2 doesn't display context menu.

What is the new behavior?
the context menu will be displayed.

Does this PR introduce a breaking change? (check one with "x")

[x] Yes
[ ] No

If this PR contains a breaking change, please describe the impact and migration path for existing applications:

Some css has to be changed due to react context menu upgrade (breaking changes were introduced from react-contextmenu)

Original ContextMenuLayer HoC is converted to ContextMenuTrigger as a component. Hence the export (wrapper of context menu in RDG) has to be updated as well.

Other information:

Qi_Li and others added some commits Dec 28, 2017

Qi_Li Qi_Li

@qili26 qili26 requested review from malonecj, amanmahajan7 and diogofcunha Jan 16, 2018

@@ -48,6 +48,7 @@ const Canvas = createReactClass({
rowScrollTimeout: PropTypes.number,
scrollToRowIndex: PropTypes.number,
contextMenu: PropTypes.element,
contextMenuId: PropTypes.string,

This comment has been minimized.

Copy link
@amanmahajan7

amanmahajan7 Jan 17, 2018

Contributor

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?

class RowsContainer extends React.Component {
constructor(props) {
super(props);
this.plugins = props.window ? props.window.ReactDataGridPlugins : window.ReactDataGridPlugins;
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);

This comment has been minimized.

Copy link
@amanmahajan7

amanmahajan7 Jan 17, 2018

Contributor

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

@@ -333,6 +334,7 @@ const Canvas = createReactClass({
width={this.props.width}
rows={rows}
contextMenu={this.props.contextMenu}
contextMenuId={this.props.contextMenuId}

This comment has been minimized.

Copy link
@amanmahajan7

amanmahajan7 Jan 17, 2018

Contributor

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

this.state = {ContextMenuContainer: this.getContextMenuContainer(props)};
this.validateContextMenu = this.validateContextMenu.bind(this);

this.validateContextMenu();

This comment has been minimized.

Copy link
@amanmahajan7

amanmahajan7 Jan 17, 2018

Contributor

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?

This comment has been minimized.

Copy link
@diogofcunha

diogofcunha Jan 18, 2018

Contributor

@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

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;

This comment has been minimized.

Copy link
@amanmahajan7

amanmahajan7 Jan 17, 2018

Contributor

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

This comment has been minimized.

Copy link
@diogofcunha

diogofcunha Jan 18, 2018

Contributor

@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

This comment has been minimized.

Copy link
@amanmahajan7

amanmahajan7 Jan 22, 2018

Contributor

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.


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

This comment has been minimized.

Copy link
@amanmahajan7

amanmahajan7 Jan 17, 2018

Contributor

toThrow/toThrowError can be used instead

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

This comment has been minimized.

Copy link
Contributor Author

qili26 commented Jan 19, 2018

@amanmahajan7
Updated, could you take another look?

}

render() {
return this.hasContextMenu() ? this.renderRowsWithContextMenu() : <SimpleRowsContainer {...this.props} />;
return this.getRowsContainer();

This comment has been minimized.

Copy link
@amanmahajan7

amanmahajan7 Jan 20, 2018

Contributor

getRowsContainer method is probably not required and the render method can just return the container

render () {
    if (this.hasContextMenu()) {
      this.validatePlugin();
      return this.renderRowsWithContextMenu();
    }

    return <SimpleRowsContainer {...this.props} />;
}
@amanmahajan7
Copy link
Contributor

amanmahajan7 left a comment

Just a small change, rest looks good

@amanmahajan7
Copy link
Contributor

amanmahajan7 left a comment

Looks good to me

@amanmahajan7

This comment has been minimized.

Copy link
Contributor

amanmahajan7 commented Jan 29, 2018

This upgrade is also required to support React 16 as react-contextmenu 1.6.x is using the deprecated APIs
https://github.com/vkbansal/react-contextmenu/releases/tag/v2.5.0

@hutber

This comment has been minimized.

Copy link

hutber commented Jan 31, 2018

Should we fix the conflicts on this and get it merged? Makes sense to merge it

@amanmahajan7

This comment has been minimized.

Copy link
Contributor

amanmahajan7 commented Jan 31, 2018

@hutber ContextMenu 2.0 has a breaking change which is preventing us from merging this PR.

@utajum utajum referenced this pull request Feb 2, 2018

Merged

add PropTypes as a separate package #954

3 of 3 tasks complete
@sontek

This comment has been minimized.

Copy link

sontek commented Feb 5, 2018

What is the breaking change? Can this PR address it?

@qili26

This comment has been minimized.

Copy link
Contributor Author

qili26 commented Feb 5, 2018

@amanmahajan7 amanmahajan7 referenced this pull request Feb 7, 2018

Merged

React 16 upgrade #1116

14 of 16 tasks complete

@malonecj malonecj changed the base branch from master to next Feb 8, 2018

@malonecj malonecj merged commit 31c9b1a into next Feb 8, 2018

2 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@sontek

This comment has been minimized.

Copy link

sontek commented Feb 13, 2018

@qili26 Ahh, is that all there is? I understand that upgrade is backwards incompatible but I think a version bump would suffice. Or are you taking extra steps to make it compatible?

@qili26

This comment has been minimized.

Copy link
Contributor Author

qili26 commented Feb 14, 2018

@sontek
I think it would be great to update this with the react v16 update together, and that's what's this PR for.
#1116

@qili26 qili26 deleted the ql-context-menu-upgrade branch Dec 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.