-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
fix(Collapse,Fade): Ensuring props don't leak to child and adding new production/limited dist builds. #598
Conversation
virgofx
commented
Sep 29, 2017
•
edited
Loading
edited
- Improved inline documentation
- Updated Readme with improved CDN and distribution file information
a8dd208
to
10f60ba
Compare
… production/limited dist builds.
@@ -1,7 +1,7 @@ | |||
import React from 'react'; | |||
import PropTypes from 'prop-types'; | |||
import classNames from 'classnames'; | |||
import Transition, { ENTERING, ENTERED, EXITING } from 'react-transition-group/Transition'; |
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.
Looks like you are removing these enums to better support when ReactTransitionGroup is not included, but can you put these in the utils and reference them from there?
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.
Yes, we can move these to utils. The main reason is that since we are cherry picking an export it prevents using the full ES6 capability for the module in production. (This gives reduced file size in the now reactstrap.full.min.js
; however, at the expense of not being able to use these.
rollup.config.js
Outdated
const umdFullConfig = baseConfig(); | ||
umdFullConfig.external = peerDependencies; | ||
umdFullConfig.plugins.push(replace({ | ||
'process.env.NODE_ENV': JSON.stringify('production'), |
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.
formatting, 2 spaces (this whole file).
@@ -0,0 +1,13 @@ | |||
#!/usr/bin/env bash |
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.
Any way to support windows <10 which do not have bash? Maybe have a node.js script and have the script in the package.json be node <file>
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.
I tried to emulate the other scripts that were there. Let me investigate a little further. If we do node file
does it autochoose the file/extension based on OS? Example of that somewhere?
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.
Those scripts don't work for me (on windows), I have to run those steps manually (which is easy since they are mostly git commands) and they only run on the "release". This is in the build, which is probably more common for others to do.
// element which results in errors/warnings for non-valid attributes. | ||
const transitionProps = pick(otherProps, TransitionPropTypeKeys); | ||
const childProps = omit(otherProps, TransitionPropTypeKeys); | ||
|
||
return ( | ||
<Transition |
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.
I would like to have a basic fallback which instantly shows/hides (no animation), but we would probably want to tie in the callbacks to that to be more compatible.
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.
At some point in future we can do better fallback whereby we don't even use the Fade elements for non fading elements. A lot more work needs to be done for that.