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

React 16 upgrade #1116

Merged
merged 41 commits into from
Feb 16, 2018
Merged

React 16 upgrade #1116

merged 41 commits into from
Feb 16, 2018

Conversation

amanmahajan7
Copy link
Contributor

@amanmahajan7 amanmahajan7 commented Feb 6, 2018

Description

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

  • Upgrade dependencies
  • Use react router 4 for the examples site
  • Remove explicit dependency on react-input-resize
  • Remove react-addons-perf as it is no longer supported
  • Fix unit tests (Migration guide)
  • Fix "Cannot have two HTML5 backends at the same time" error in Drag/Drop examples. This only happens when "Draggable header" example is opened after the "Row Reordering" example and vice versa.
  • Remove fbjs . Facebook recommends against using this utility "If you are consuming the code here and you are not also a Facebook project, be prepared for a bad time."

Please check if the PR fulfills these requirements

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

[ ] Bugfix
[x] 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)

What is the new behavior?

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: ...
React Context Menu: #1081

Other information:

@malonecj malonecj changed the base branch from master to next February 8, 2018 12:54
@amanmahajan7 amanmahajan7 removed the WIP label Feb 9, 2018
@@ -6,10 +6,7 @@ const config = {
'react-data-grid-addons/dist/react-data-grid-addons': ['./packages/react-data-grid-addons/src'],
'react-data-grid/dist/react-data-grid.min': ['./packages/react-data-grid/src'],
'react-data-grid-addons/dist/react-data-grid-addons.min': ['./packages/react-data-grid-addons/src'],
'react-data-grid-examples/dist/shared': './packages/react-data-grid-examples/src/shared.js',
'react-data-grid-examples/dist/examples': './packages/react-data-grid-examples/src/examples.js',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A single index page is now being used instead of three separate pages (index, examples, and documentation)

@@ -86,7 +86,7 @@ describe('Grid', function() {
};

this.componentWrapper = this.createComponent();
this.component = this.componentWrapper.node;
this.component = this.componentWrapper.instance();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

node is a private property and an error is thrown in V3

@@ -1,95 +0,0 @@
import ReactPerf from 'react-addons-perf';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed these tests as react-addons-perf is no longer supported

.keyDown({
keyCode: letterEKeyCode
}, gridRunner.cell )
.resetCell(coords)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Caching does not work in V3 so resetting cell after each operation

@@ -113,7 +113,8 @@ class FocusableComponentTestRunner {
expect(this.componentPrototype.focus).not.toHaveBeenCalled();
});

it('should focus when scrolling and selected', () => {
// TODO: why is this test failing?
xit('should focus when scrolling and selected', () => {
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 does not seem like a correct test. focusableComponentWrapper does not update if the selection is not changed and when isScrolling state is change this does not trigger a re-render so focus is never called. Is the focusableComponentWrapper working as expected? I am also not sure how was this test passing at the first place, may be because of the experimental lifecycleExperimental flag

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 we should remove this test, rather than ignoring it. I dont think it is correct either

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 the test


Enzyme.configure({
adapter: new Adapter(),
disableLifecycleMethods: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need this flag to keep v2 behavior.

import { DragDropContext } from 'react-dnd';
import HTML5Backend from 'react-dnd-html5-backend';

export default DragDropContext(HTML5Backend);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

react-dnd throws and an error if DragDropContext is initialized with HTML5Backend twice

@amanmahajan7 amanmahajan7 removed the WIP label Feb 16, 2018
@amanmahajan7 amanmahajan7 merged commit 83f7f3e into next Feb 16, 2018
@amanmahajan7 amanmahajan7 deleted the am-react-16 branch February 16, 2018 18:18
@cwiese
Copy link

cwiese commented Mar 26, 2018

Is this done? Are there are issue?

@colindjk colindjk mentioned this pull request Jul 18, 2018
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants