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.

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

TabPane with unmountOnExit should retain transition #3497

Closed
sisp opened this issue Feb 23, 2019 · 10 comments
Closed

TabPane with unmountOnExit should retain transition #3497

sisp opened this issue Feb 23, 2019 · 10 comments

Comments

@sisp
Copy link
Contributor

sisp commented Feb 23, 2019

I would have expected TabPane to retain the transition even when unmountOnExit is true, but looking at the the source null is returned immediately rather than when onExited is triggered. Is this intended?

@jquense
Copy link
Member

jquense commented Feb 23, 2019

It should retain the transition, active isn't toggled to false until the transition calls it's onExited callback, at which point we render null so it's unmounted

@sisp
Copy link
Contributor Author

sisp commented Feb 23, 2019

I can't confirm that the transition is happening and unless I'm missing anything I don't see how it should happen given the source code.

The following snippet shows how the active prop is determined from the context:

active:
  props.active == null && key != null
    ? makeEventKey(activeKey) === key
    : props.active,

The <Transition /> component animates the transition when the active prop changes. But when unmountOnExit === true the <Transition /> component is not rendered with active === false because null is returned immediately (see here).

@jquense
Copy link
Member

jquense commented Feb 23, 2019

Sorry are you saying it is working but you don't understand why or it's not working?

@sisp
Copy link
Contributor Author

sisp commented Feb 23, 2019

It's not working for me and the source code doesn't look like it's doing the right thing either.

@jquense
Copy link
Member

jquense commented Feb 24, 2019

Ok probably a bug then

@sisp
Copy link
Contributor Author

sisp commented Feb 24, 2019

I've put together a small example based on the custom tabs layout example from the docs: https://codesandbox.io/s/xl5n993np4

Change the unmountOnExit prop of the App component to see the difference and notice that unmountOnExit === true prevents animated transitions.

@jquense
Copy link
Member

jquense commented Feb 25, 2019

yup you're right, I think that one line should be !active && !Transition && unmountOnExit

fungjj92 added a commit to fungjj92/react-bootstrap that referenced this issue Aug 8, 2019
@fungjj92
Copy link
Contributor

Bump! Hi project maintainers. Can we get the proposed fix above in? I can make a PR if you'd like. I actually started, but thought I'd bump this issue first before putting in too much time to accumulate PR detail. Happy to do whatever is most helpful! Thanks.

fungjj92 added a commit to fungjj92/react-bootstrap that referenced this issue Aug 21, 2019
@taion
Copy link
Member

taion commented Aug 21, 2019

We'd be happy to take a PR here.

fungjj92 added a commit to fungjj92/react-bootstrap that referenced this issue Aug 22, 2019
fungjj92 added a commit to fungjj92/react-bootstrap that referenced this issue Aug 30, 2019
The docs incorrectly suggest that the transition prop accepts the value 'true' to 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.

Refs react-bootstrap#3497
fungjj92 added a commit to fungjj92/react-bootstrap that referenced this issue Mar 12, 2020
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
fungjj92 added a commit to fungjj92/react-bootstrap that referenced this issue Mar 12, 2020
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
taion pushed a commit that referenced this issue Mar 13, 2020
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 #3497
@taion
Copy link
Member

taion commented Mar 13, 2020

fixed in #4312

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

No branches or pull requests

5 participants