-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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] Add v5 banner #27070
[docs] Add v5 banner #27070
Conversation
Details of bundle changes.Comparing: feaad0a...05646f2 Details of page changes
|
Co-authored-by: Matt <[email protected]>
Co-authored-by: Matt <[email protected]>
Co-authored-by: Sebastian Silbermann <[email protected]>
Co-authored-by: Sebastian Silbermann <[email protected]>
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.
See https://v3.material-ui.com/getting-started/installation/ for a previous iteration on a banner #15514, what I think that we are missing:
- The font size is too large: body1-> body2
- The background color in dark mode can be grey
- We need to offset the page, the padding-top of the headers, sidenav, etc.
In the future we could consider:
- A close button
- To leave the infrastructure in place to have other important announcement
docs/translations/translations.json
Outdated
@@ -85,6 +85,9 @@ | |||
"toggleRTL": "Toggle right-to-left/left-to-right", | |||
"toggleTheme": "Toggle light/dark theme", | |||
"traffic": "Traffic", | |||
"v5IsOut": "v5 beta is out! 🎉 Head to the", |
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.
As a side note, I think that it would have been OK to not translate these keys. We have paused the sync anyway with Crowdin.
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.
Regarding some of Olivier's points:
The font size is too large: body1-> body2
The background color in dark mode can be grey
It definitely feels better with a smaller type and a little bit more padding. And also agree that the primary dark color doesn't work so well on dark mode. I've used the palette.background.paper on this tweak here.
Also, changed the emoji to the beginning just so v5 isn't too lonely there starting out.
A close button
Don't think it's needed, I believe that the type of announcements that are placed on a banner like this, persistent on the home page, are the ones we'd want to have on there for a given time-box so we can promote the news as loud as we can. We could and should make it look better though when we revamp the documentation design (maybe for the stable rollout of v5?!), the current Material Design look doesn't make me that excited about v5 😅
To leave the infrastructure in place to have other important announcement
Definitely!
display: 'block', | ||
padding: 4, | ||
textAlign: 'center', | ||
backgroundColor: theme.palette.primary.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.
Don't feel like this one is working too well on dark mode... I've tried using palette.background.paper
and it worked better.
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 we use the current for light mode and the grey for 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 think it seems to work fine?!
@@ -39,7 +39,7 @@ const useStyles = makeStyles( | |||
flex: '1 0 100%', | |||
}, | |||
hero: { | |||
paddingTop: theme.spacing(8), | |||
paddingTop: theme.spacing(8) + 30, |
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.
If it was going to HEAD, we could have asked for a const, but since it's for a legacy branch, YOLO 👍
We forgot to account for the anchor offset: https://material-ui.com/guides/minimizing-bundle-size/#option-1 I have done a quick fix: 125f3db. |
Preview: https://deploy-preview-27070--material-ui.netlify.app/