-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
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][material-ui] Add script to generate template screenshots #42903
Conversation
Netlify deploy previewhttps://deploy-preview-42903--material-ui.netlify.app/ Bundle size report |
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.
Looks good. I just left few questions because I'm not familiar with this part of the codebase.
@@ -60,7 +60,9 @@ function ToggleCustomTheme({ showCustomTheme, toggleCustomTheme }) { | |||
<AutoAwesomeRoundedIcon sx={{ fontSize: '20px', mr: 1 }} /> | |||
Custom theme | |||
</ToggleButton> | |||
<ToggleButton value={false}>Material Design 2</ToggleButton> | |||
<ToggleButton id="toggle-default-theme" value={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.
I'm wondering if id="toggle-default-theme"
could confuse new user, thinking this id
has an impact on the theme
<ToggleButton id="toggle-default-theme" value={false}> | |
<ToggleButton data-screen-shots="toggle-default-theme" value={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.
That's a lot better. I changed to data-screenshot="toggle-mode"
and data-screenshot="toggle-default-theme"
.
// eslint-disable-next-line no-console | ||
console.log(error); | ||
} |
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.
Not sure it changes something
// eslint-disable-next-line no-console | |
console.log(error); | |
} | |
// eslint-disable-next-line no-console | |
console.error(error); | |
} |
href: '/material-ui/getting-started/templates/dashboard/', | ||
source: `${sourcePrefix}/docs/data/material/getting-started/templates/dashboard`, | ||
hasDarkMode: true, |
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.
Should it also get a hasDefault
to allow showing images with -default.jpg
and -default-dark.jpg
?
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'd be overkill for now. It can be a separate PR but need help from @zanivan to find a good DX to show between custom/default with light/dark.
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 plan to use the default image in a blog post (separate PR), that's the reason I added it.
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.
Design-wise, it looks great—Thanks for tackling this!
Leaving my approval here for you to merge after addressing Alex's feedback :)
Part of #41469
Motivation
Reduce manual work to update the screenshots when code changes.
The script is for local use, not intend to run on CI.
Further improvements