-
Notifications
You must be signed in to change notification settings - Fork 244
Conversation
Codecov Report
@@ Coverage Diff @@
## master #73 +/- ##
==========================================
+ Coverage 86.2% 86.25% +0.04%
==========================================
Files 15 15
Lines 319 320 +1
Branches 42 42
==========================================
+ Hits 275 276 +1
Misses 44 44
Continue to review full report at Codecov.
|
@baldwmic I build the files as UMD. So root property is used for exposing to window global. It should remain ['React', 'addons', 'CSSTransitionGroup'] because I just checked the latest build from https://unpkg.com/[email protected]/dist/react-with-addons.js and it has |
@baldwmic I will take a look this PR and merge. It may take some time. |
@brijeshb42 i've updated the PR per your comment about the build. Thanks for taking a look and let me know if I've missed anything! |
webpack.config.js
Outdated
root: ['React', 'addons', 'CSSTransitionGroup'], | ||
commonjs2: 'react-transition-group', | ||
commonjs: 'react-transition-group', | ||
amd: 'react-transition-group', | ||
} |
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.
Can you change this to
{
'react-transition-group/CSSTransitionGroup': {
root: ['React', 'addons', 'CSSTransitionGroup'],
commonjs2: 'react-transition-group/CSSTransitionGroup',
commonjs: 'react-transition-group/CSSTransitionGroup',
amd: 'react-transition-group/CSSTransitionGroup',
}
}
Otherwise the whole package is included in the build file which significantly increases the file size.
Everything else looks good and I'll merge once the changes are done.
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.
@brijeshb42 I've updated the PR per your comment
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.
definitely wouldn't want to increase build size unnecessarily
@brijeshb42 yup! just updated the PR |
@baldwmic Just published v0.5.1 after merging your PR. |
Why?
react
is a peer dependency for this project (as you correctly state in Update to React 15.5 #67), some of the internal code for the library itself relies on old versions of React.What?
Prop-Types
React.PropTypes
toprop-types
package using the codemodprop-types
a dependency as recommended by ReactCSS Transition Group
react-addons-css-transition-group
toreact-transition-group
Enzyme
react-dom/test-utils
instead of oldreact-addons-test-utils
Question:
React
in the build as described here. But I'm not sure theroot
property is the right way to do it.What are your thoughts on this @brijeshb42 ?