Skip to content

Conversation

@taion
Copy link
Member

@taion taion commented Apr 17, 2020

No description provided.

@taion taion requested review from jquense and silvenon and removed request for silvenon April 17, 2020 03:18
Copy link
Collaborator

@silvenon silvenon left a comment

Choose a reason for hiding this comment

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

Awesome! ❤️

Two problems I spotted:

  • yarn storybook fails because rules.js.inlineCss from webpack-atoms is no longer a function
  • I think this is a good opportunity to revisit two tests in TransitionGroup-tests.js which define the Child component
    • these tests seem invalid because componentWillEnter and componentWillLeave don't exist 🤷

@taion
Copy link
Member Author

taion commented Apr 17, 2020

d'oh

@taion
Copy link
Member Author

taion commented Apr 18, 2020

@silvenon okay, the storybook was screwed up more broadly because the transitions themselves were disabled. i fixed it and modernized some of the code. i also took out those 2 stale test cases

@taion taion requested a review from silvenon April 18, 2020 04:23
Copy link
Collaborator

@silvenon silvenon left a comment

Choose a reason for hiding this comment

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

Looks great! 👌 Nice an modern ❤️

@silvenon silvenon merged commit bf4cd7c into master Apr 18, 2020
@silvenon silvenon deleted the up branch April 18, 2020 07:06
@unrevised6419
Copy link

unrevised6419 commented Apr 18, 2020

@taion @silvenon My suggestion would be to revert back react and react-dom devdependencies versions to

"react": "~16.6.3",
"react-dom": "~16.6.3",

because they are listed also in peerDependencies, and there is a risk to use some react APIs in released package that are not supported by the dependencies of react-transition-group users, and this would imply a breaking change.

@taion
Copy link
Member Author

taion commented Apr 18, 2020

The peer dependencies use a >= range. I expect most users are actually on the latest React versions at this point, so it seems to me to make more sense to test against those.

@unrevised6419
Copy link

unrevised6419 commented Apr 18, 2020

I expect most users are actually on the latest React versions at this point

I also thought this on redux-form but it seems is not the case. 🙂

What I mean above is that for example I'm working on a feature in react-transition-group and I will be tempted to use a react feature that was introduced in react 16.8 (my build will succeed, because dev version of react is 16.13). And we release this new feature. And some users of the library, may update to latest minor version but still have react 16.6 installed, and their build will fail.

@silvenon
Copy link
Collaborator

I think it's best to set up a sanity test on CI with lowest supported React version.

@jquense
Copy link
Collaborator

jquense commented May 5, 2020

🎉 This PR is included in version 4.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants