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

[docs-infra] Fix hydration api #39706

Merged
merged 1 commit into from
Nov 12, 2023

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Nov 1, 2023

I have noticed this while I was looking at #39704.

Before http://0.0.0.0:3000/material-ui/api/dialog/

Screenshot 2023-11-02 at 00 48 19

After: no more hydration warning. https://deploy-preview-39706--material-ui.netlify.app/material-ui/api/dialog/

A continuation of #35201.

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work scope: docs-infra Specific to the docs-infra product labels Nov 1, 2023
@mui-bot
Copy link

mui-bot commented Nov 1, 2023

Netlify deploy preview

https://deploy-preview-39706--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against b8d19ac

Comment on lines +227 to +233
{disableAd ? null : (
<BrandingProvider>
<AdGuest>
<Ad />
</AdGuest>
</BrandingProvider>
)}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the two other <Ad /> are used with the same wrapper. Would it make sens to rename Ad by AddContent

And define <Ad /> as follow:

const Ad = ({ disableAd }) => {
  if (disableAd) {
    return null;
  }
  
  return  <BrandingProvider>
              <AdGuest>
                <AddContent />
              </AdGuest>
            </BrandingProvider>
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could bundle <AdGuest> and <Ad> together, it makes sense.

I wouldn't bundle <BrandingProvider> because this one shouldn't be here. I think we nest way too many ThemeProviders in our docs, I suspect part of why pages are this slow. I think that one theme provider higher in the tree for the docs itself, and one theme provider per demo is enough.

@oliviertassinari oliviertassinari merged commit 5b58896 into mui:master Nov 12, 2023
22 checks passed
@oliviertassinari oliviertassinari deleted the docs-infra-api-ad branch November 12, 2023 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work scope: docs-infra Specific to the docs-infra product
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants