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] Improve the left side-nav #22780

Merged
merged 4 commits into from Oct 4, 2020

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Sep 27, 2020

It was an opportunity to work on 3 problems at the same time:

1. Fix w3 validator

The warning was present on all the pages and was buried all the other issues (x81)

Capture d’écran 2020-09-27 à 19 47 17

https://validator.w3.org/nu/?doc=https%3A%2F%2Fnext.material-ui.com%2Fcomponents%2Fslider-styled

There are still 3 issues related to navigation (edit page, previous page, and next page) but that's for another day.

2. Update the design

It's closer to the Material Design specification https://material.io/components/navigation-drawer. I also wanted the design to better support more levels of nesting and more items.

Before
Capture d’écran 2020-09-27 à 19 45 22

After
Capture d’écran 2020-10-04 à 14 10 01

I have used the following as inspiration too:

3. Improve the rendering performance

I was getting frustrated at the time it takes to open the drawer on mobile and the time it takes to click on TOCs hash link. In the following benchmark in development, opening the drawer is about x2 faster. The main change is to use fewer React components.

Before
Capture d’écran 2020-09-27 à 19 58 16

After
Capture d’écran 2020-09-27 à 19 59 04e

The performance for changing the hash is also improved but less significantly as the rendering time of the main body of the page stays the same. In dev:

Before
Capture d’écran 2020-09-27 à 20 03 02

After
Capture d’écran 2020-09-27 à 20 02 28

@oliviertassinari oliviertassinari added the docs Improvements or additions to the documentation label Sep 27, 2020
@mui-pr-bot
Copy link

mui-pr-bot commented Sep 27, 2020

Details of bundle changes

Generated by 🚫 dangerJS against 96ea691

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Using our components should be preferred for dog-feeding purposes. If you profile for performance you should always use the production build not development. It is apparent from your screenshot that you used the development build. This is why we have #15282

I quickly did a run with https://deploy-preview-22780--material-ui.netlify.app/ vs https://next--material-ui.netlify.app/ and meassured 90ms (pr) vs 120ms (next) when opening with 6x CPU throttling.

The performance gain is negligent compared to the usefulness of dog-feeding.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Sep 27, 2020

The performance gain is negligent

@eps1lon Yeah, it seems that the performance gain of reducing the usage of components decreases significantly between dev and production. I'm happy to learn that! Thanks for testing it :). I was lazy (didn't want to wait for a build on my local env but rather use Netlify). Agree, I don't think that its a game-changer, only a nice free win.

the usefulness of dog-feeding

What is there to dog feed? From what I understand, we were using the List component that is meant to display a list of items on mobile: https://material.io/components/lists for navigation. This usage is questionable if not wrong.
Hopefully, the new style could be later transformed into standalone https://material.io/components/navigation-drawer components. We track this in #15107.

@mbrookes
Copy link
Member

mbrookes commented Oct 3, 2020

Not keen on the lack of a hover indicator on the top level items; and with no focus indicator, keyboard navigation is impossible.

@oliviertassinari
Copy link
Member Author

@mbrookes Updated

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.

None yet

4 participants