-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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] Allow to pass navigation bar banner from outside #36299
Conversation
Netlify deploy previewhttps://deploy-preview-36299--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.
👍
children: PropTypes.node.isRequired, | ||
className: PropTypes.string, | ||
disableDrawer: PropTypes.bool, | ||
}; | ||
|
||
AppFrame.defaultProps = { | ||
BannerComponent: AppFrameBanner, |
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 set enable_docsnav_banner
to false
to hide the current banner given the upcoming v6 stable release?
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.
Yes, that's a good point. By the way, do you think it will improve adaption a bit by keep showing this banner (after updating text v6-beta
-> v6
) on core docs for a few weeks after stable? After a short period we can remove it from core docs and only keep it in v5 X-specific docs. What do you think?
Side note: I noticed that this banner is not displayed for smaller width screens i.e. < 1200px
(when the sidebar disappears), this is probably to keep UI for mobile screens consistent and single liner but I feel some desktop displays also fall under the same screen resolution range.
I am not sure how much a percentage of users are on smaller displays, but adding this on smaller ones will possibly improve its visibility a bit.
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.
do you think it will improve adaption a bit by keep showing this banner (after updating text v6-beta -> v6) on core docs for a few weeks after stable?
That's a good idea for sure!
Although we have to make sure that the v6 stable banner only appears after the release is done.
We can keep the current banner, and then update it as soon as the v6 stable is available and deploy it to production (new instructions in #36301 could be helpful for the deployment process)
I am not sure how much a percentage of users are on smaller displays
In the last 12 months, the breakdown looks like this:
- desktop: 97.73%
- mobile: 2.19%
- tablet: 0.09%
I would not focus too much on adding this banner on mobile.
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.
👍 will see how to make it easier in the future.
Motivation
After a stable MUI X release, in MUI X v5 docs, a notification for v6 is needed to be shown similar to the one here for v5.
Exposing a prop from
AppLayoutDocs
which receivesBanner
component will allow to:v5.mui.com
andmui.com
will point to the same version of MUI Core but different of MUI X, so adding a banner like today won't do the need full.)This PR will be accompanied by update in docs-v5 adding banner on MUI-X stable release.