-
Notifications
You must be signed in to change notification settings - Fork 208
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
Update to React 0.14 #79
Conversation
bfdd97d
to
3da3452
Compare
@AlanFoster Take a look and let me know what you think! |
Still waiting on Travis integration to be enabled unfortunately, so running tests is a manual process for now. #83 is keeping track of this however. After a rebase on master, are all tests green? |
3da3452
to
50778b6
Compare
Ah, I see - are there instructions for how to run the tests? |
@rolyatmax I've just updated the readme file now under #86. Ping me if you need any help |
@AlanFoster et al Running tests - a couple failing tests which all seem to be related to changes in My best guess is that they have something to do with this line: https://github.com/onefinestay/react-daterange-picker/blob/master/src/calendar/tests/CalendarDate.spec.js#L64 All tests using the I don't have a ton of experience with using React's Test Utils though. Any thoughts? Pushing up my latest, and here's the test output:
|
50778b6
to
95e67c0
Compare
Also, haven't been able to get tests passing on current |
@rolyatmax They unfortunately fail currently because the To get the tests running with 0.14.x however; Would you be okay updating this.renderedComponent = TestUtils.findRenderedDOMComponentWithTag(renderedTable, 'td'); to be this.renderedComponent = ReactDOM.findDOMNode(TestUtils.findRenderedComponentWithType(renderedTable, CalendarDate)); |
Can do! I'll submit updates shortly! :-)
|
@rolyatmax Awesome, thanks :) |
@AlanFoster Hmmm - that doesn't seem to be working for me. Have you pulled this down and tried those changes? Sorry I'm not more help - I'm not terribly familiar with the TestUtils. |
@rolyatmax @AlanFoster It looks like TestUtils.renderIntoDocument now returns a DOM node (react#4692), and findRenderedComponentWithType and findRenderedDOMComponentWithTag both expect a ReactComponent to traverse. Playing around with it on my own, I had success simply using getElementsByTagName on the rendered this.renderedComponent = renderedTable.getElementsByTagName("td")[0]; I also noticed the 'creates extra lis based on the props.stateDefinitions' test in the Legend spec was failing. Doing the following seemed to fix it: var lis = ReactDOM.findDOMNode(this.renderedComponent);
expect(lis.childNodes.length).toBe(3);
var spans = lis.childNodes[1].getElementsByTagName('span');
expect(spans.length).toBe(2);
expect(spans[0].style.backgroundColor).toBe('blue');
expect(spans[1].textContent).toBe('abc'); |
95e67c0
to
ff170b1
Compare
❤️ ❤️ @thathenderson ❤️ ❤️ This is great. Thanks!! Squashed some updates into that last commit. Tests and linting both pass.
@AlanFoster Let me know what you think |
Any chance we can get this merged in? |
Testing this has been complicated for me by unrelated React 0.14-upgrade stuff elsewhere in my project's code-base; but as much as I am able to test, this PR seems good for me and worth merging. 👍 |
expect(lis.length).toBe(3); | ||
var spans = TestUtils.scryRenderedDOMComponentsWithTag(lis[1], 'span'); | ||
|
||
var lis = ReactDOM.findDOMNode(this.renderedComponent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Won't this.renderedComponent
already be a DOM Node at this point; can we delete the findDOMNode
call?
Sorry, as @AlanFoster pointed out, I missed something. renderIntoDocument returns a DOM node only when the rendered component is wrapped in a DOM node. That's why I originally included findDOMNode. You can refactor it to remove findDOMNode and the ReactDOM dependency if you wrap this.useDocumentRenderer = (props) => {
const domComponent = TestUtils.renderIntoDocument(<div>{getLegend(props)}</div>);
this.renderedComponent = domComponent.childNodes[0];
}; |
ff170b1
to
5267a4c
Compare
@AlanFoster @thathenderson Take a look at that updated last commit and let me know how that looks. All tests and linting are passing. |
@rolyatmax Could you rebase against master please, there's a small merge conflict to resolve |
5267a4c
to
bbde573
Compare
@AlanFoster rebased |
@rolyatmax Perfect, thanks. All green on travis now too 👍 |
@AlanFoster 👍 👍 👍 👍 |
bbde573
to
4ac5dca
Compare
Rebased again to resolve conflicts. All tests and linting pass. |
Any chance for a merge? Looking to upgrade our version of React, but this is holding us back |
+1 for merge :) We're also upgrading to React 0.14, and this is the only dependency holding us back. |
For visibility I plan to merge #99 before releasing 0.12.2, then I shall be merging this PR with the intent of a 1.0.0 release. Would appreciate a rebase for this PR 👍 |
4ac5dca
to
889f574
Compare
@AlanFoster rebased! |
@rolyatmax can you rebase again? Looks like 0.12.2/1.0.0 are close to be ready |
889f574
to
85488c4
Compare
Rebased - and added a commit to remove an unused var in the gulpfile to pass linting. Tests/linting both pass. |
Thanks for all the work on this PR guys, v0.12.2 is now released - so let's get this merged in now! 👍 |
Can you push to NPM too? |
Splitting this out from #78.