Updates to React 16 #231
Updates to React 16 #231
Conversation
| @@ -53,7 +53,7 @@ exports[`Badge should return content inside container if title not provided 1`] | |||
| </div> | |||
| `; | |||
|
|
|||
| exports[`Badge should return noscript if children and title not provided 1`] = `<noscript />`; | |||
| exports[`Badge should return noscript if children and title not provided 1`] = `""`; | |||
There was a problem hiding this comment.
You could test return null cases without snapshot. Just do this: expect(component.type()).toEqual(null);
There was a problem hiding this comment.
Oh, I see you changed that. So that way should work here as well, right?
There was a problem hiding this comment.
Added assertion to verify that it returns null.
There was a problem hiding this comment.
Cool, thanks! ...but do we still really need to compare to snapshot? I would suggest to not create snapshots in these cases and just check the type()/html(). How do you think guys?
There was a problem hiding this comment.
Strangely, the type() did not work... Don't know why, though.
Regarding the comparison to the snapshot... Well, for the null cases I guess no.
| @@ -39,6 +40,6 @@ export default class CalendarWrapper extends React.Component { | |||
| } | |||
|
|
|||
| CalendarWrapper.propTypes = { | |||
| initialDates: React.PropTypes.arrayOf(String), | |||
| minDate: React.PropTypes.string, | |||
| initialDates: PropTypes.arrayOf(String), | |||
There was a problem hiding this comment.
Not part of your changes but this can't be right, right?
There was a problem hiding this comment.
You mean the fact that we have propTypes on a wrapper/mock or the fact that I ordered the propTypes?
There was a problem hiding this comment.
arrayOf(String) should be arrayOf(PropTypes.string). The first one will basically accept an array of truthy values.
There was a problem hiding this comment.
@reaktivo I think the arrayOf receives the Constructor/Type. How would you do with a custom class/Component? PropTypes.MyComponent?
There was a problem hiding this comment.
Ahh screw it, I do think this is supported, but anyway... Changing 🙄
There was a problem hiding this comment.
Now I got it... It's not a PropType checker... just a constructor ... I thought the arrayOf would do (internally) value instanceof ConstructorThatWasPassedInTheArrayOf.
Codecov Report
@@ Coverage Diff @@
## master #231 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 41 41
Lines 1051 1051
Branches 261 261
=====================================
Hits 1051 1051
Continue to review full report at Codecov.
|
mAiNiNfEcTiOn
force-pushed
the
update-react-16
branch
from
e15eae6
to
710a2e9
Compare
…Replaces enzyme-adapter-react-15 with enzyme-adapter-react-16. Updates Jest to 21.3.0-beta to get a fix for the rAF warning
…places the usage of enzyme-adapter-react-15 for enzyme-adapter-react-16
… Updates snapshots. Removes the usage of React.PropTypes in favour of 'prop-types' one
….spec. Updates the snapshots too
…ovided and the badge is not visible
mAiNiNfEcTiOn
force-pushed
the
update-react-16
branch
from
5b597d7
to
0d113d2
Compare
|
@mAiNiNfEcTiOn should we delete branch? |
What does this PR do:
enzyme-adapter-react-15withenzyme-adapter-react-16both inpackage.jsonandtests/setupTests.js.jestto `21.3.0-betaª to get a fix for the rAF warning (see more at: Add simple rAF polyfill in jsdom environment jestjs/jest#4568 (comment) )rafpackage, replacing the existing one.<noscript />fornull.renderTree.html()'s result and also removes the usage ofReact.PropTypesin favour ofprop-typesone.Where should the reviewer start: