Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update react transition group #515

Merged
merged 8 commits into from
Mar 1, 2019
Merged

Update react transition group #515

merged 8 commits into from
Mar 1, 2019

Conversation

nLight
Copy link
Contributor

@nLight nLight commented Feb 26, 2019

No description provided.

@nLight
chore: add react keys
a7de8e2
@nLight
feat: update react-transition-group@2.5.0
32878ed 
BREAKING CHANGE: transitionLeaveTimeout -> transitionExitTimeout
@nLight
feat: update react-transition-group@2.5.0
09fc33f 
BREAKING CHANGE: transitionLeaveTimeout -> transitionExitTimeout
@nLight nLight force-pushed the dima/update-react-transition-group branch from 32878ed to 09fc33f Compare February 26, 2019 18:10
@TattdCodeMonkey
Copy link
Contributor

This looks good, but should it be a feat commit or a BREAKING CHANGE?

TattdCodeMonkey
TattdCodeMonkey approved these changes Feb 26, 2019
View reviewed changes
Copy link
Contributor

@TattdCodeMonkey TattdCodeMonkey left a comment

Choose a reason for hiding this comment

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

Other than the comment about if this should be a breaking change, this looks good. I tested this in dcos-ui using npm link and every worked as expected.

@nLight
Copy link
Contributor Author

nLight commented Feb 26, 2019

@TattdCodeMonkey it's both:

feat: update react-transition-group@2.5.0

BREAKING CHANGE: transitionLeaveTimeout -> transitionExitTimeout

The only problem with the change is that all transitions are gone... so something is missing. either css or js setup

@TattdCodeMonkey TattdCodeMonkey mentioned this pull request Feb 26, 2019
Merged
nLight
nLight commented Feb 28, 2019
View reviewed changes
src/Dropdown/Dropdown.js Outdated Show resolved Hide resolved
nLight
nLight commented Feb 28, 2019
View reviewed changes
src/Dropdown/Dropdown.js Outdated Show resolved Hide resolved
@nLight
Copy link
Contributor Author

nLight commented Feb 28, 2019

@TattdCodeMonkey

TLDR couldn't figure out List ListItem though
dropdown transitions but fails to detect if it should be opening up
modal works
this is how it should be http://mesosphere.github.io/reactjs-components/

@nLight
Copy link
Contributor Author

nLight commented Feb 28, 2019

testing npm run livereload

@mperrotti
Copy link
Contributor

About the List/ListItem transition in:
I see we updated the styles for a list item animating out, but I don't see any changes for animating in.

However, I think the slide in animation is slow and unnecessary, and has a negative impact on perceived performance. I recommend we remove the ListItem transition.

@TattdCodeMonkey
fix(Dropdown): rendering up
b939d30
@TattdCodeMonkey
refactor(List): remove transition group
6267cb2 
When upgrading to react-transition-group v2.5 the transitions in List &
ListItem were broken. While working on fixing them we decided to just
remove the transitions instead. This library is depracated, we're not
aware of any usages of this component and we don't really like the
transition here anyway.
@TattdCodeMonkey TattdCodeMonkey force-pushed the dima/update-react-transition-group branch from 390e395 to 6267cb2 Compare February 28, 2019 19:41
@mperrotti
Copy link
Contributor

Getting a few console errors from props being passed from form components to their dom nodes. A good example is textarea.
To be fair, this was happening before these changes, so I won't let that block this particular PR.

@GeorgiSTodorov GeorgiSTodorov merged commit 7af2d2b into master Mar 1, 2019
@GeorgiSTodorov GeorgiSTodorov deleted the dima/update-react-transition-group branch March 1, 2019 08:54
@mesosphere-frontend-ci
Copy link
Contributor

🎉 This PR is included in version 5.0.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
Reviewers

@TattdCodeMonkey TattdCodeMonkey TattdCodeMonkey approved these changes

@mperrotti mperrotti mperrotti approved these changes

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@nLight @TattdCodeMonkey @mperrotti @mesosphere-frontend-ci @GeorgiSTodorov