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

[Accordion] renders components even when it's closed. #10569

Closed
behivetech opened this issue Mar 7, 2018 · 20 comments
Closed

[Accordion] renders components even when it's closed. #10569

behivetech opened this issue Mar 7, 2018 · 20 comments
Labels
component: accordion This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process. new feature New feature or request

Comments

@behivetech
Copy link

behivetech commented Mar 7, 2018

Does this library render components even when they are hidden? For example, ExpansionPanel. It appears that this component renders everything inside even when it's closed by default. What if you have components inside of there that have extensive functionality going on and you had 20 expansion panels doing the same thing and the user only opened one of them? Doesn't this seem inefficient running through functionality of several components that may never even be seen by the user?

I am aware that I could potentially look at the onClick function and find out if the panel is expanded or not then render the component, but it seems like this should be a default behavior of the ExpandedPanel or any component that show/hides components.

  • [ x] I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior

Only render components inside the ExpansionPanel when it's open. Unmount components inside the ExapansionPanel when it is closed.

Current Behavior

Renders component regardless if the ExpansionPanel is open or closed.

Steps to Reproduce (for bugs)

Check out the console.logs for the sandbox below. They kick off regardless of the ExpansionPanel being closed.
https://codesandbox.io/s/lyo2m9ky0q

@oliviertassinari oliviertassinari added component: accordion This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process. labels Mar 8, 2018
@oliviertassinari
Copy link
Member

@jbsmith969 This issue shares some root cause with #10183. You are free to change the behavior. The following option disables the rendering of the component in the DOM.

<ExpansionPanel CollapseProps={{ unmountOnExit: true }}>

https://codesandbox.io/s/p9z3jk639x

it seems like this should be a default behavior of the ExpandedPanel or any component that show/hides components.

This is an interesting point to discuss. It's a tradeoff. By not mounting the hidden content you will make the content unreachable for SEO and increase the responsiveness as more work is going to be needed by the browser to display the content at the click interaction. On the other hand, mounting everything makes to initial reconciliation of the page slower.
I'm happy to change the default behavior of the ExpansionPanel. Do you want to submit a pull request?

@oliviertassinari oliviertassinari added the new feature New feature or request label Mar 8, 2018
@Loktor
Copy link

Loktor commented Mar 8, 2018

I would like to chip in here, i have a usecase where it is benefitial to have to content rendered all the time to make form submits easier (even if they are not visible). It's a settings page where the expansionPanel is used to seperate the form into sections (where some details are most of the time not really important).
Would be great if there would be a solution where it is up to the user to decide if the expansion panel content should render initially (i guess unmountOnExit is this option already?).
Besides that, if the use case of not rendering at first is more often used then it probably makes sense to make this the default behaviour and adapt the documentation to inform users about this.

@jascoul
Copy link

jascoul commented Mar 8, 2018

@oliviertassinari I had the same issue, and the CollapseProps works for me, thanks. Personally, I think not rendering children unless the panel is open is more intuitive.

@behivetech
Copy link
Author

Hey @oliviertassinari! Thanks for the response. Sorry, I must have missed that unmountOnExit prop when searching through docs. Do you think we could make that name a little more intuitive since it seems to also prevent rendering from your description? That may have been why I missed it. I'll give that prop a shot to see if that works. If I can find some time, I'll try to make some changes to the code and make a pull request. I'm leaning towards not rendering children by default and a prop to render children for cases like SEO and what @Loktor brought up.

@oliviertassinari oliviertassinari self-assigned this Mar 8, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 8, 2018

Would be great if there would be a solution where it is up to the user to decide if the expansion panel content should render initially

@Loktor I agree, it's a problem we have already been facing and solved on the Modal side: #10572.

Besides that, if the use case of not rendering at first is more often used then it probably makes sense to make this the default behavior and adapt the documentation to inform users about this.

Yes, I do think that the unmounting should be the default behavior. You will most likely render many items with the ExpansionPanel component. It can greatly improve the initial rendering this way.

Sorry, I must have missed that unmountOnExit prop when searching through docs.

@jbsmith969 You won't find this key in our documentation. It's a property implemented by react-transition-group library.

If I can find some time, I'll try to make some changes to the code and make a pull request.

Oh great! I think that we should implement the same approach than with the Modal: a keepMounted property. I will let you the lead on this issue :).

@oliviertassinari oliviertassinari removed their assignment Mar 8, 2018
@oliviertassinari oliviertassinari added this to the post v1.0.0 milestone Mar 8, 2018
@TomBillingtonUK
Copy link

If no one has picked it up yet , I'd like to give this one a go

@mib00038
Copy link

mib00038 commented Jul 8, 2018

@oliviertassinari Don't we need to set {mountOnEnter: true} to prevent an ExpansionPanel from rendering hidden child components when it's first mounted? I don't see the point of using unmountOnExit here for most use cases.

mountOnEnter
By default the child component is mounted immediately along with the parent Transition component. If you want to "lazy mount" the component on the first in={true} you can set mountOnEnter. After the first enter transition the component will stay mounted, even on "exited", unless you also specify unmountOnExit.

type: boolean
default: false

unmountOnExit
By default the child component stays mounted after it reaches the 'exited' state. Set unmountOnExit if you'd prefer to unmount the component after it finishes exiting.

type: boolean
default: false
http://reactcommunity.org/react-transition-group/transition

@salientknight
Copy link

If you use the unmount option, is the content still on the page for Readers? If the content is not on the page, then this is not an accessible option. I solved this yesterday but looping through the links and toggling the tabIndex between -1 and 0, which seems to have the proper functioning, but it would be nice if this behavior was not only baked in but default.

@ralvs
Copy link
Contributor

ralvs commented Jul 26, 2019

Just an update, on version 4.2.1 uses TransitionProps

<ExpansionPanel TransitionProps={{ mountOnEnter: true }}>

@eps1lon
Copy link
Member

eps1lon commented Jul 27, 2019

We've added a section to our docs describing different tradeoffs:

The content of ExpansionPanels is mounted by default even if the panel is not expanded. This default behavior has server-side rendering and SEO in mind. If you render expensive component trees inside your panels or simply render many panels it might be a good idea to change this default behavior by enabling the unmountOnExit in TransitionProps: <ExpansionPanel TransitionProps={{ unmountOnExit: true }} />. As with any performance optimization this is not a silver bullet. Be sure to identify bottlenecks first and then try out these optimization strategies.

-- https://material-ui.com/components/expansion-panels/#performance

@oliviertassinari oliviertassinari changed the title ExpansionPanel renders components even when it's closed. [Accordion] renders components even when it's closed. Jul 18, 2020
@odedbendov
Copy link

Thanks everyone for your amazing contribution! An idea to get the best of both worlds in this case:
unmountOnExit could be accompanied by a renderClosedContentForSEO or something of the rather. The function would build a long simple string with all the SEO juice goodness we want, and be hidden in some div, so only crawlers see it.

This way we don't render complicated DOM that is hidden, but do publish all the content we want for the crawlers

WDYT?

@oliviertassinari
Copy link
Member

@odedbendov This sounds like cloaking, not great, but it can be implemented by unmounting the accordion when hidden, and rendering something else in place, visually hidden.

@MABGuiza
Copy link

Any plans of having the same feature available for Joy-ui?
Or is there an elegant way of unmounting on exit?

@CalvinJamesHeath
Copy link

CalvinJamesHeath commented Feb 13, 2024

The child components still render even when the Accordion is not expanded on [email protected].

@mib00038
Copy link

mib00038 commented Feb 13, 2024

@CalvinJamesHeath
yes, child components are rendered but visibility and overflow are hidden
I believe this allows for the show/hide animation (as apposed to display: none) ?
Are you experiencing any unexpected behaviour ?

@CalvinJamesHeath
Copy link

@mib00038
I have big component tree nested inside many accordions, and its unefficitent because it slows down the app performance, is there a way to only render the accordion details when its expanded but without breaking the animation transition?

because if I add some simple logic inside the accordion like

<Accordion>
//code
{expaned === 'config' && (
<AccordionDetails>
//components
</AccordionDetails>
)}
<Accordion />

But that unfortunately breaks the smooth expanding transition.

@mib00038
Copy link

mib00038 commented Feb 13, 2024

@CalvinJamesHeath
As per the API docs here

If you render the Accordion Details with a big component tree nested inside, or if you have many Accordions, you may want to change this behavior by setting unmountOnExit to true inside the slotProps.transition prop to improve performance:

<Accordion slotProps={{ transition: { unmountOnExit: true } }} />

Also try moving your expaned logic into the Accordion expanded prop:
ie <Accordion expanded={expaned === 'config'} >

Does that help ?

@CalvinJamesHeath
Copy link

Yeah I already had all that set. The AccordionDetails childs also get mounted even though the accordion is not expanded. I wanted to see if there was a way not to mount them to increase performance.

@pibahamondesw
Copy link

I agree with @CalvinJamesHeath , I think it might be related to this error message:

Warning: React does not recognize the `slotProps` prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase `slotprops` instead. If you accidentally passed it from a parent component, remove it from the DOM element.

Chrome suggests to use instead slotprops, which of course doesn't work. I was working with MUI v.5.11.2 and using

TransitionProps={{ unmountOnExit: true }}

works fine, but on v.5.11.15, this is deprecated and slotProps is not doing its job when using the suggested

slotProps={{ transition: { unmountOnExit: true } }}

A similar issue was reported here:
mui/mui-x#10072

@AtanasVA
Copy link

@CalvinJamesHeath As per the API docs here

If you render the Accordion Details with a big component tree nested inside, or if you have many Accordions, you may want to change this behavior by setting unmountOnExit to true inside the slotProps.transition prop to improve performance:

<Accordion slotProps={{ transition: { unmountOnExit: true } }} />

Also try moving your expaned logic into the Accordion expanded prop: ie <Accordion expanded={expaned === 'config'} >

Does that help ?

Setting slotProps={{ transition: { unmountOnExit: true } }} worked perfectly in my case, thanks a ton! 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: accordion This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process. new feature New feature or request
Projects
None yet
Development

No branches or pull requests