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

[Collapse] content should exist in DOM before expanded #16945

Closed
2 tasks done
mlenser opened this issue Aug 9, 2019 · 4 comments
Closed
2 tasks done

[Collapse] content should exist in DOM before expanded #16945

mlenser opened this issue Aug 9, 2019 · 4 comments
Labels
component: transitions This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@mlenser
Copy link
Contributor

mlenser commented Aug 9, 2019

Based on the discussion for ExpansionPanel: #10569 the contents of the Collapse element should also be in the DOM before it is expanded. For SEO reasons and the other reasons outlined in that thread.

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

The Card component demo uses Collapse and the content of the collapse does not appear in the DOM until it is expanded.

Expected Behavior 🤔

The Collapse element should render the content in the DOM like the ExpansionPanel does: https://material-ui.com/components/expansion-panels/#performance

Steps to Reproduce 🕹

Card component demo

@oliviertassinari
Copy link
Member

@mlenser I'm not sure to fully follow. Are you suggesting this diff?

diff --git a/docs/src/pages/components/cards/RecipeReviewCard.tsx b/docs/src/pages/components/cards/RecipeReviewCard.tsx
index 4ec8bf40a..95081c256 100644
--- a/docs/src/pages/components/cards/RecipeReviewCard.tsx
+++ b/docs/src/pages/components/cards/RecipeReviewCard.tsx
@@ -94,7 +94,7 @@ export default function RecipeReviewCard() {
           <ExpandMoreIcon />
         </IconButton>
       </CardActions>
-      <Collapse in={expanded} timeout="auto" unmountOnExit>
+      <Collapse in={expanded} timeout="auto">
         <CardContent>
           <Typography paragraph>Method:</Typography>
           <Typography paragraph>

There is one thing that comes to my mind, I think that the performance section of the expansion panels panel would find a great home under the /components/transitions/ page of the documentation.

In #16935, we are discussing going in the other direction for the tree view. I think that SEO is a concern for about 15% of our users (people using the SSR API). It's not the majority. It shouldn't be our main concern. In the future, if we can apply the hidden prop to the transition element and React implement its fiber engine, the performance difference between the two will be, most likely, negligible on the client. I have no objection in using the default behavior in this case (mount everything).

@oliviertassinari oliviertassinari added component: transitions This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process. labels Aug 9, 2019
@mlenser
Copy link
Contributor Author

mlenser commented Aug 9, 2019

Thanks for taking the time to look at my issue Olivier,

Ah, then I misunderstood that other issue. It sounded like SEO was important based on your comment on the issue and #10569 (comment). I understand that ~15% use it and it's not a priority, though it's great that the team has made it an option for those who need it, thanks for that!

I see that unmountOnExit on the Collapse does exactly what I was expecting. That's good to know! If that's the default you want I'm happy to modify the docs to make it the default.

Could we also add unmountOnExit to the Collapse API (https://material-ui.com/api/collapse/)? Then others could find it as well

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 9, 2019

@mlenser The prop is already documented (in react-transition-group) in the API page. But it's not visible enough. I think that the most important concern here is to better explain the situation in /components/transitions/. What do you think? Do you want to contribute a new Performance & SEO section? :)

Well, I used to work on an SEO intensive product using Material-UI when designing v1. So, I was forced to consider it, hence my bias. The good news is that with technologies like Next.js and Gatsby and the flow of plain JS, HTML and CSS users coming to React, this is a growing concern :).

@mlenser
Copy link
Contributor Author

mlenser commented Aug 9, 2019

The prop is already documented (in react-transition-group) in the API page

I never seem to get there. I now see that there is a link from the Collapse component so I probably should've found it but somehow I never do. I generally just look at Props and CSS and ignore everything else.

The ideal situation would be to add props to the props table, but then that'd requiring querying another library and that'd be crazy to expect. I'm surely a lazy developer so this can be ignored as I have no meaningful ways to improve its visibility.

I think that the most important concern here is to better explain the situation in /components/transitions/

I rarely end up on that page. What you have now is sufficient and developers should just be better about reading.

Thanks again for your time! I'll close the issue as everything works as expected once I meaningfully look at the docs.

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

No branches or pull requests

2 participants