-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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
[docs] List all the Tab components under the API section #21241
Conversation
to avoid re-render of tab sub-components on every select of tabs, instead of creating tab sub-components, thier visibility can be modified.
It's meant to speed-up perf. You are free to customize the rendering, I don't think that we should change the demo. |
In this way, the state held in TabPanel's child controls get lost between tab switches. |
Details of bundle changes.Comparing: 9bd4277...8dda088 Details of page changes
|
@emretapci Yes, it's part of the tradeoff and why I was proposing a new prop to TabPanel. |
@oliviertassinari what will this new prop do? |
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.
This changes semantics. Before you wouldn't mount the children if the tabpanel wasn
't visible. Now you do.
It's not obvious that this is the better default.
In a component's render function, if you conditionally create a component, when that sub-component does not get created, its state is lost. In the current case of the tab demo, if the tab panel is not visible, its children are not rendered. So, its state is lost. Since ReactJS is a declarative system, maybe we should avoid rendering components conditionally, especially if they hold a state that needs to be retained. Nevertheless, it took some of my time to notice this fact, since I am a newbie in React :) |
I'm aware that state is lost. It's still not clear that this is the correct approach since you now render all tab panels which could be considered wasteful. If you want to persist state you could always lift it up and put into context or whatever you prefer for state managment. In the future React will likely provide better support for offscreen rendering. For this demo conditional rendering is sufficient. Though we might want to add an explainer why the current behavior was chosen and how to deal with stateful tab panels. |
@eps1lon I would propose we move the focus of this pull request from the demos to a focus on Using the following benchmark list https://trello.com/c/JM6Zh3YW/2642-tabs (using the same one that previously), I could find the following behaviors:
This echo to oliviertassinari/react-swipeable-views#453 (comment). Rendering all the tab panels reduces the time it takes to switch to a different tab panel but increases the initial mount. What do you guys think about going with 4, plus an option to only render the active panel (it can be useful to reduce the SSR output, even with low priority rendering of the |
I don't see how this relates to a docs PR. |
@eps1lon I believe we are discussing what should the tab panel mounting behavior be. @emretapci's problem is about persisting state between panels, he has proposed to change the documentation to account for his case. I think that we need to be aware of the other constraints before making any change. In addition to the state persistence concern, I think that the performance is also a legit problem to take into account. I'm opening a new issue: #21250. |
@emretapci I have updated your pull request with a small change related to the documentation. I hope it will help in this problem: making the lab API more visible might increase usage, which in turn, might drive developers to #21250 and your problem. Thanks for raising your voice. |
to avoid re-render of tab sub-components on every select of tabs, instead of creating tab sub-components, their visibility can be modified.