-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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: Allow tab to un/mount with transition #4312
fix: Allow tab to un/mount with transition #4312
Conversation
7996b7c
to
334d304
Compare
would be good to add a test here |
Thanks for your speedy response, meanwhile pardon my delays! I agree a test would be good. I'm not familiar with enzyme. Ideally, I'd be able to test the className progression (the entering and exiting phases) to ensure the transition is being applied. I don't get the sense this can be done, rather a test will only be able to naively check the final classNames. Please advise. Thanks! |
Take a look at e.g. react-bootstrap/test/ToastSpec.js Line 54 in b81eb86
|
5877f31
to
b1b1777
Compare
Also, fix documentation errors: - Tab component docs stated that the transition prop of value 'true' would use the 'Fade' transition. Rather, the 'Fade' transition is applied by default. Otherwise, the transition prop accepts 'false' or a react-transition-group 'Transition' component. - Replace references to v2 with v4 Refs react-bootstrap#3497
b1b1777
to
231743a
Compare
I was reminded this PR was still hanging out 🐌 I just rebased and pushed changes. I had taken a look at writing a test, which I agree would be helpful, but struggled to fake the desired behavior. Would gladly accept a test commit to this branch if it is deemed necessary. |
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.
well, i guess i'm okay with this for now.
the non-transition case is a bit funky in that we shouldn't mount this until the first render if mountOnEnter
is false, but that's not a regression per se
I tested it and the fix works. The lack of a test is a bit unfortunate, however we do not currently have tests for animation of tabs as far as I can tell. We could probably merge this fix and add the test case to the ongoing issue #3889 ? |
Since react-bootstrap#4312, marking a TabPane as `unmountOnExit` will have had the same effect as `mountOnEnter`, i.e. that the component is not there at all when the tab is not active. This makes `mountOnEnter` have the same effect. Fixes react-bootstrap#5048
…nsition Since react-bootstrap#4312, marking a TabPane as `unmountOnExit` will have had the same effect as `mountOnEnter`, i.e. that the component is not there at all when the tab is not active. This adds full support mount-on-enter and unmount-on-exit also when not using a Transition. Fixes react-bootstrap#5048
Overview
The Tabpane component has a bug where when set to unmount on exit, any transitions are ignored. The solution was already discussed in issue #3497 by @sisp and @jquense ; I am shepherding it to the library.
Demo
@sisp created a codesandbox showing how in the latest release of react-bootstrap
<TabPane ... unmountOnExit=true />
on does not execute the transition between tabs.Refs #3497