-
-
Notifications
You must be signed in to change notification settings - Fork 31.7k
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
[docs-infra] Fix layout shift ad #37694
[docs-infra] Fix layout shift ad #37694
Conversation
Netlify deploy previewhttps://deploy-preview-37694--material-ui.netlify.app/ Bundle size report |
6d3ae6c
to
a409062
Compare
m: (theme) => theme.spacing(4, 0, 3), | ||
mt: 4, | ||
mb: 3, |
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.
Clean up
@@ -111,7 +111,7 @@ class AdErrorBoundary extends React.Component { | |||
} | |||
} | |||
|
|||
function Ad() { | |||
export default function Ad() { |
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.
Convention
@@ -47,7 +47,7 @@ const StyledAppContainer = styled(AppContainer, { | |||
...(!disableAd && { | |||
...(!hasTabs && { | |||
'&& .description': { | |||
marginBottom: 198, | |||
marginBottom: 4 * 10 + 3 * 10 + 126, |
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.
The fix 1/2
// TODO, remove this prop, the layout should be retained as much as possible | ||
// between pages, to improve performance, and retain UI states, e.g. scroll | ||
disableLayout = false, |
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.
Clean up, I don't understand how the creation of AppLayoutDocsWithoutAppFrame.js
would be the right thing to do.
padding: `${theme.spacing(1.5)} ${theme.spacing(1.5)} ${theme.spacing( | ||
1.5, | ||
)} calc(${theme.spacing(1.5)} + 130px)`, | ||
padding: '12px 12px 12px calc(12px + 130px)', |
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.
Retain the same style values, after changing from spacing.unit 8px to 10px.
}, [theme]); | ||
|
||
// TODO: remove MdThemeProvider, top level layout should render the default theme. |
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.
I think that this is really important, we want to show the default look in the docs.
66cbb05
to
153b85a
Compare
153b85a
to
01e10ff
Compare
01e10ff
to
002fa21
Compare
This was broken at one point, the UX is not great, there are 2 layout shifts:
Screen.Recording.2023-06-25.at.00.03.14.mov
https://mui.com/joy-ui/getting-started/overview/
It's so smooth now 馃槍 https://deploy-preview-37694--material-ui.netlify.app/joy-ui/getting-started/. I think that it matters because we want the ad to blend into the content, the less it's distinguishable, the less developers will avoid it.