-
-
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] Remove icons and tweak the design of the side nav #37860
Conversation
Really nice! Simpler and easier to see subheaders. Should we also apply this to X and Toolpad? |
Netlify deploy previewhttps://deploy-preview-37860--material-ui.netlify.app/ Bundle size report |
I like this a lot! Makes the subheaders much easier to distinguish from the docs themselves. |
Love this! 🎉 Looks so much cleaner! |
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.
LGTM!
So I believe we should remove all the icons from the sidenavs, e.g. Line 9 in 4036eb6
It's bundle size / code we don't use. MUI X might be the most impacted by this change, but no objections from my end. This is a different tradeoff: which I imagine can be solved with an index page, like https://mui.com/base-ui/all-components/. |
@@ -1,6 +1,5 @@ | |||
import * as React from 'react'; |
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.
@danilo-leal Nice, I love the new side nav design! 🎉
I tried it out in MUI X - where we have a more nested structure: https://deploy-preview-9716--material-ui-x.netlify.app/x/react-data-grid/
Here are a few things I have noticed so far:
- Pages on the same level are not visually aligned. It looks like "Columns" and "Rows" are children of the "Layout" page, even though they are siblings:
- No vertical separator on the second level of nesting.
Do you think it would look better if there were separators on each nesting level?
Let me know what you think 🙂
Looking forward to bringing the new design to MUI X docs!
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.
It sounds like we should add this navigation case in https://master--material-ui.netlify.app/experiments/docs/installation/, so that when we make changes, we can also cover nested navigation.
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.
@cherniavskii sweet, I'm glad you enjoyed it, and thanks for calling attention to this ⎯ @oliviertassinari had already signaled it! Is there a way I can play with this to try to tackle these issues? I don't know how I'd change something on this repo and have an effect on the X docs 🤔 Having a mocked scenario on the experiments folder would 100% help, I think!
Either way, apologies for not having consulted y'all before merging that PR, that's my bad 😞
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 can add use cases from MUI X to https://master--material-ui.netlify.app/experiments/docs/installation/ 👍
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.
@cherniavskii that'd be perfect! Let me know when these are up so I can promptly work on the design!
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.
@danilo-leal Here you go: #38047
After a few days of sleeping on this, I'm enjoying this design. It removes some of the visual weight of the whole docs, simplifies things, and makes it easier to identify subheaders in the side nav. Here's how it looks:
https://deploy-preview-37860--material-ui.netlify.app/base-ui/getting-started/