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

[Tabs] Improve tab panels mounting behavior #21250

Open
oliviertassinari opened this issue May 30, 2020 · 24 comments
Open

[Tabs] Improve tab panels mounting behavior #21250

oliviertassinari opened this issue May 30, 2020 · 24 comments
Labels
component: tabs This is the name of the generic UI component, not the React module! new feature New feature or request

Comments

@oliviertassinari
Copy link
Member

oliviertassinari commented May 30, 2020

Based on #21241, we might have an opportunity to improve the TabPanel mounting behavior. We can potentially fix these problems:

  • Lost states between different panels (by keeping the panels mounted)
  • Improve the performance of panel transitions (by mounting the panels ahead of time)
  • Improve the SEO support (by rendering all the panels' content)

I have used the following benchmark list https://trello.com/c/JM6Zh3YW/2642-tabs to look at what the others are doing

  1. hidden prop with full rendering and no option to disable it https://github.com/reach/reach-ui/blob/115eb680bae13e7e097c9a6cd3047c06530ee880/packages/tabs/src/index.tsx#L643
  • Reach UI
  • Reakit
  • Garden
  • Chakra
  • React Bootstrap
  1. lazy render the content of the tab panels, keep them mounted when moving to a different tab
  • Antd (plus a forceRender prop to render all the panels)
  1. only render the active tab panel
  • Base Web (plus a renderAll prop to render all the panels)
  • Material-UI
  • JetBrains
  • Semantic UI
  • AltasKit
  • Polaris
  1. render in two passes, first the active panel, then all the panels
  • react-swipeable-views

In the future, React mentions maybe solving this in the framework: oliviertassinari/react-swipeable-views#453 (comment), by providing better support for offscreen rendering.

What do you guys think about these different tradeoffs? It seems that we need, at least, a way to enable the mounting of all the panels.

@Zloka
Copy link

Zloka commented Oct 12, 2020

Hey! Really liking Material UI, but supporting the unmounting behavior described here would be very desirable. Any updates on this? :)

@ambroseus
Copy link

ambroseus commented Nov 11, 2020

@oliviertassinari I use simple solution to have always mounted TabPanels:

export function TabPanel(props: any) {
  const {
    children,
    className,
    style,
    value: id,
    Container = DefaultContainer,
    ...other
  } = props
  const context = useTabContext()

  if (context === null) {
    throw new TypeError('No TabContext provided')
  }
  const tabId = context.value

  return (
    <div
      className={className}
      style={{
        width: '100%',
        ...style,
        position: 'absolute',
        left: 0,
        visibility: id === tabId ? 'visible' : 'hidden',
      }}
      {...other}
    >
      <Container>{children}</Container>
    </div>
  )
}

@shayantabrizi
Copy link

No news on this? Aren't upvotes enough for being considered?

@oliviertassinari
Copy link
Member Author

@shayantabrizi Feel free to work on this problem.

@dnquang1996vn
Copy link

dnquang1996vn commented Jun 9, 2021

@oliviertassinari I use simple solution to have always mounted TabPanels:

export function TabPanel(props: any) {
  const {
    children,
    className,
    style,
    value: id,
    Container = DefaultContainer,
    ...other
  } = props
  const context = useTabContext()

  if (context === null) {
    throw new TypeError('No TabContext provided')
  }
  const tabId = context.value

  return (
    <div
      className={className}
      style={{
        width: '100%',
        ...style,
        position: 'absolute',
        left: 0,
        visibility: id === tabId ? 'visible' : 'hidden',
      }}
      {...other}
    >
      <Container>{children}</Container>
    </div>
  )
}

This solution load all tabs at begin, kind of

1.hidden prop with full rendering and no option to disable it

If we want to make

2.lazy render the content of the tab panels, keep them mounted when moving to a different tab

We can add a checker and change a bit like this

export function TabPanel(props: any) {
  const {
    children,
    className,
    style,
    value: id,
    Container = DefaultContainer,
    ...other
  } = props
  const context = useTabContext()
 
  if (context === null) {
    throw new TypeError('No TabContext provided')
  }
  const tabId = context.value

  const [visited, setVisited] = useState(false)
  useEffect(() => {
    if(id === tabId) {
      setVisited(true)
    }
  }, [id, tabId]);

  return (
    <div
      className={className}
      style={{
        width: '100%',
        ...style,
        position: 'absolute',
        left: 0,
        visibility: id === tabId ? 'visible' : 'hidden',
      }}
      {...other}
    >
       {visted && <Container>{children}</Container>}
    </div>
  )
}

@eps1lon
Copy link
Member

eps1lon commented Jun 10, 2021

If anybody wants to make a deep dive into React 18 then I would suggest investigating wrapping the panel content in React.Suspense. Eventually you'll want to use an upcoming Offscreen API for the hidden TabPanels so React can de-prioritize the hidden panel (and persist state when it gets hidden). There are also interesting questions like selective hydration for hidden TabPanels that would be enabled by React.Suspense and how this behaves when the user starts navigating to a TabPanel before it is hydrated.

I don't have a full picture for this yet hence why I suggest that somebody investigates this area since I believe tabs can really make use of React 18.

Edit:

In the future, React will likely provide better support for offscreen rendering

I think it's safe to assume that this will happen.

@kodeschooler
Copy link

kodeschooler commented Oct 7, 2021

@oliviertassinari I use simple solution to have always mounted TabPanels:

export function TabPanel(props: any) {
  const {
    children,
    className,
    style,
    value: id,
    Container = DefaultContainer,
    ...other
  } = props
  const context = useTabContext()

  if (context === null) {
    throw new TypeError('No TabContext provided')
  }
  const tabId = context.value

  return (
    <div
      className={className}
      style={{
        width: '100%',
        ...style,
        position: 'absolute',
        left: 0,
        visibility: id === tabId ? 'visible' : 'hidden',
      }}
      {...other}
    >
      <Container>{children}</Container>
    </div>
  )
}

@ambroseus
Could you please show the code where you're creating the useTabContext ? I'm just a little unclear how this prevents the hidden component from being unmounted. Whether you use <div hidden> or <div class="visibility: hidden"> the component still unmounts. I must be missing something, but I'd like to see what you're storing in context in addition to value. And how using context along with visibility hidden produces the desired effect. thanks

@ambroseus
Copy link

@kodeschooler
https://github.com/ambroseus/mui-tab-panel-demo

@loetek
Copy link

loetek commented Jan 25, 2022

@dnquang1996vn Did you mean to say
const [visited, setVisited] = **React.useState(false)**

instead of

const [visited, setVisited] = **useEffect(false)**

@dnquang1996vn
Copy link

@loetek Yeah i mean this. Thanks for checking me. I updated comment

@ievgennaida
Copy link

ievgennaida commented Jun 16, 2022

@oliviertassinari
Could you please check the latest version of the lab tabpanel.
It's causing child components to be mounted two times.

Just add to any tab panel content this line:

React.useEffect(()=> console.log('mounted')),[])

every time you switch a tab, mounting is done two times.
This is causing to all the ajax calls to run twice and etc.

@MukeZ
Copy link

MukeZ commented Jun 23, 2022

Same here, I use official example, It always calls twice when I switch tabs.

@hoangtrung99
Copy link

hoangtrung99 commented Jun 24, 2022

@ievgennaida @MukeZ
What version of React are you using?
I think your problem is related to New Strict Mode Behaviors in React 18.

@MukeZ
Copy link

MukeZ commented Jun 24, 2022

@hoangtrung99
Yes I'm using React 18, It only renders once when I remove <React.StrictMode>.
And it renders twice when StrictMode on.

@ievgennaida
Copy link

@ievgennaida @MukeZ What version of React are you using? Is the problem related to New Strict Mode Behaviors in React 18?

It's a strict mode of react is causing this.

@bsides
Copy link

bsides commented Oct 6, 2022

The example given here isn't very good for me as I couldn't use absolute positioning of the tabs in any way. Is there a more unobtrusive way?

@cjauvin
Copy link

cjauvin commented Oct 23, 2022

I'm new to MUI (currently using version 5.6) and I'd like to add my humble perspective on this issue, as a newcomer. The current behavior of the MUI Lab TabPanel, as far as I understand it, rather contradicts my a priori mental model of how a set of UI tabs should work: it took me a while just to realize that the reason my internal state was always reset was because the panel unmounts when going invisible, and mounts again when coming back into view (for a while I thought I did not understand something deeper with React hooks, until I had the idea to put a console.log in the return function of useEffect, which made me click).

I understand that this makes sense performance-wise, and how I can work around it by putting my state elsewhere (using Redux or a parent component for instance), but still, for my use case, which is to have data grids with many controlling state variables inside each panel, I find that solution not terribly satisfying. So if there was an option to at least control that behavior, it would be ideal in my opinion.

I will take the occasion to add that the reason why there are two sets of components, to implement tabs (one from MUI Core, the other from the Lab), is not very clear to me, when I read the documentation. The pros and cons of using each is also not fully clear, although I understand that by using the MUI Core set I will have greater control to implement the behavior I need, so that's what I will try next.

@Soundvessel
Copy link

Soundvessel commented Jan 6, 2023

How would an iframe in a tab be affected by the Offscreen API? Would that document still function and update as if it was simply in another tab?

We are currently using the modified TabPanel code above because we have legacy UI being brought into tabs and the user needs to freely switch between them without losing their place. Not ideal, but unfortunately for those of us working with legacy, an iframe can be a desired way of encapsulating old functionality. And I believe some services still utilize iframes or DOM injection via JS which could require similar behavior.

@YGKtech
Copy link

YGKtech commented Feb 18, 2023

Just commenting to voice my support for implementing better behavior for tabs, or at least making the docs more clearly address their behavior. I had no idea they unmounted components like this until after I'd sunk a fair bit of time into a layout based around them.

It's obviously unexpected behavior for a layout component to have, and not trivial to work around, so it should either be fixed, or the behavior should be called out loudly in the docs.

@MaybeThisIsRu
Copy link

MaybeThisIsRu commented Feb 23, 2023

@oliviertassinari I use simple solution to have always mounted TabPanels:

export function TabPanel(props: any) {
  const {
    children,
    className,
    style,
    value: id,
    Container = DefaultContainer,
    ...other
  } = props
  const context = useTabContext()

  if (context === null) {
    throw new TypeError('No TabContext provided')
  }
  const tabId = context.value

  return (
    <div
      className={className}
      style={{
        width: '100%',
        ...style,
        position: 'absolute',
        left: 0,
        visibility: id === tabId ? 'visible' : 'hidden',
      }}
      {...other}
    >
      <Container>{children}</Container>
    </div>
  )
}

This is not a solution for a majority of cases. If your tab content is of variable height (very likely), you'll get stuck again on account on absolute positioning. Once you go absolute, your content will no longer have a natural height. You'll be forced to resort to annoying JS hacks to get the height working again.

The best "workaround" at the moment is to keep your state one level up more than you need it. Build stateless tab content components and keep the state in the component/function that's rendering the TabContext. When MUI has a native solution, refactor this component/function. Yes, this is not ideal, I understand that, but it's the best we can make of what we have at the moment.

@xcollantes
Copy link

Would there be an fix for this available for the Tabs Component?

I have an example of code here to recreate the issue: https://codesandbox.io/s/amazing-margulis-brfny5

@httol
Copy link

httol commented May 25, 2023

Would there be an fix for this available for the Tabs Component?

I have an example of code here to recreate the issue: https://codesandbox.io/s/amazing-margulis-brfny5

you can use redux to restore the value

@yewool0818
Copy link

yewool0818 commented Sep 14, 2023

@ambroseus

Thanks for sharing good way. when i use that code, I found a potential but fatal bug.
I use <DataGrid> component in props.children component.

When I use same code that you posted, there is a problem that didn't remove FooterSelectedRowCount element.
(That's class are MuiDataGrid-selectedRowCount and css-de9k3v-MuiDataGrid-selectedRowCount)
It might be MUI bug.

also, visibility doesn't change the pre page's height and wide, It's not look nice.

so I suggest display instead of visibility.

// visibility: id === tabId ? 'visible' : 'hidden',
display : id === tabId ? 'block' : 'none'

@liuhenry
Copy link

liuhenry commented Oct 26, 2023

There are some cases where other MUI components are constructed in a way that prevents the external state workaround.

For example, with controlled row selection in DataGrid, the re-render itself triggers onRowSelectionModelChange, so the value gets cleared even when the state/setState is external:

const [rowSelectionModel, setRowSelectionModel] = React.useState<
  GridRowSelectionModel
>([]);

...

<TabPanel value={0}>
  <ControlledSelectionGrid
    rowSelectionModel={rowSelectionModel}
    setRowSelectionModel={setRowSelectionModel}
  />
</TabPanel>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: tabs This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

No branches or pull requests