Skip to content
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] Update the nav order #28323

Merged
merged 9 commits into from
Sep 15, 2021
Merged

[docs] Update the nav order #28323

merged 9 commits into from
Sep 15, 2021

Conversation

mbrookes
Copy link
Member

@mbrookes mbrookes commented Sep 13, 2021

preview: https://deploy-preview-28323--material-ui.netlify.app/components/buttons/

This PR updates the order of the components in the nav to something slightly more logical:

Components

  • Inputs
  • Data display
  • Feedback
  • Surfaces
  • Navigation
  • Layout
  • Utils
  • Data grid
  • Lab

This roughly flows from the "inside" to the "outside" of a UI, and with the most common / popular components first.

The main exceptions are Data grid, which is a data display component, but isn't in core; and Lab. Utils has always been an odd one out, since they aren't all components... I could also see Surfaces and Navigation being swapped, but no strong preference.

I also think we should remove the padding between list items. The nav is too long already:

image

This also makes navigation feel snappier not having the dead space between items when moving the pointer.

@mui-pr-bot
Copy link

mui-pr-bot commented Sep 13, 2021

No bundle size changes

Generated by 🚫 dangerJS against 59eff7b

@eps1lon eps1lon changed the base branch from next to master September 14, 2021 08:46
@mbrookes mbrookes added the docs Improvements or additions to the documentation label Sep 14, 2021
Copy link
Contributor

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with it, it makes sense for me!

Co-authored-by: danilo leal <[email protected]>
@eps1lon eps1lon changed the title [Docs] Update the nav order [docs] Update the nav order Sep 15, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 15, 2021

I also think we should remove the padding between list items. The nav is too long already:

Fair enough, but could we "skew" the nav item header to more clearly recognize what's the header of what?

Current

Screenshot 2021-09-15 at 11 37 49

Proposal

Screenshot 2021-09-15 at 11 45 55

diff --git a/docs/src/modules/components/AppNavDrawerItem.js b/docs/src/modules/components/AppNavDrawerItem.js
index f2cae9359b..04a419f91e 100644
--- a/docs/src/modules/components/AppNavDrawerItem.js
+++ b/docs/src/modules/components/AppNavDrawerItem.js
@@ -130,7 +130,7 @@ const ItemButton = styled(Item, {
     })(),
     fontSize: depth === 1 ? theme.typography.pxToRem(12) : theme.typography.pxToRem(14.5),
     fontWeight: depth === 1 ? 700 : 500,
-    margin: theme.spacing(0.5, 0),
+    margin: depth === 0 ? theme.spacing(0.5, 0) : '8px 0 4px',
     '&:hover': {
       backgroundColor: depth === 0 ? '' : alpha(theme.palette.primary.main, 0),
       color: (() => {

@oliviertassinari
Copy link
Member

As a side note, on

Screenshot 2021-09-15 at 11 53 12

We could consider grey 600 instead of 500 to improve the contrast of the subheader.

@mnajdova
Copy link
Member

We could consider grey 600 instead of 500 to improve the contrast of the subheader.

Agree 👍

@mnajdova
Copy link
Member

I am mering this in order to include it before the stable release.

@mnajdova mnajdova merged commit 1a7f454 into mui:master Sep 15, 2021
fontWeight: depth === 1 ? 700 : 500,
margin: theme.spacing(0.5, 0),
fontSize: theme.typography.pxToRem(depth === 0 ? 14.5 : 12),
fontWeight: depth === 0 ? 600 : 700,
Copy link
Member

@oliviertassinari oliviertassinari Sep 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we said we wouldn't use 600? #28327. In #28323 (comment) I mean for the color, not the font-weight 👼 .

I guess it's another example where not using design tokens can cost us.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, great catch. We aren't importing the 600 font-weight, only 400, 500, and 700. 600 ends up rendering the 700.

oliviertassinari added a commit to oliviertassinari/material-ui that referenced this pull request Sep 16, 2021
oliviertassinari added a commit to oliviertassinari/material-ui that referenced this pull request Sep 16, 2021
oliviertassinari added a commit to oliviertassinari/material-ui that referenced this pull request Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants