mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.51k stars 32.19k forks source link

Add NProgress component #22486

Open oliviertassinari opened 4 years ago

oliviertassinari commented 4 years ago

Summary 💡

Provide a component to display a progress bar with Next.js, Gatsby, etc.

Examples 🌈

import * as React from 'react';
import NProgressBar from '@material-ui/core/NProgressBar';

Router.onRouteChangeStart = () => {
  NProgress.start();
};

Router.onRouteChangeComplete = () => {
  NProgress.done();
};

Router.onRouteChangeError = () => {
  NProgress.done();
};

function AppFrame(props) {
  return (
    <NProgressBar />
  )
}

Sep-04-2020 17-49-59

Motivation 🔦

On Chrome mobile, a progress bar is displayed by default. On Chrome desktop, no progress bars are displayed. However, a number of popular websites are using an approach similar to Next.js and Gatsby. You can find YouTube 32B sessions/month, Facebook 25B sessions/month.

As it turns out, we already have this component available under @material-ui/docs/NProgressBar and use it for our documentation. We could introduce this component in the lab, with a couple of improvements, and kill the docs npm package. The improvements we could bring:

Benchmark

oliviertassinari commented 3 years ago

A bug we will likely want to solve at the same time. The progress with the dark mode of the docs can barely be seen:

Light ✅

Capture d’écran 2020-10-12 à 21 40 55

Dark ❌

Capture d’écran 2020-10-12 à 21 40 32
italodeandra commented 3 years ago

Hey, I can work on this one!

I'll start a PR tomorrow and I hope I have something to show by the end of the day. Like you said, nprogress is simple and we can mimic it's behavior. I have done some thinking design-wise and perhaps you can share you thoughts too.

The progress bar from nprogress has a glowing tip. Does it make sense to copy that too? Or perhaps we should keep it Material and mimic Chrome's that doesn't have it and it's just a LinearProgress?

-- Edit

Perhaps we could add a standalone customization to the documentation like we have with other components like TextField, Stepper, etc.

oliviertassinari commented 3 years ago

Note that there is a NPorgress component in the @material-ui/docs package that can yield some extra context.

For the design, I guess sticking to YouTube/GitHub or another popular implementation could be enough.

For the theme customization, would using the default prop be enough?

italodeandra commented 3 years ago

Note that there is a NPorgress component in the @material-ui/docs package that can yield some extra context.

Yes, I'm doing some investigation on all the links you added to the Benchmark.

For the design, I guess sticking to YouTube/GitHub or another popular implementation could be enough.

Okay, I agree. YouTube/Chrome mobile is mostly just a LinearProgress fixed on the top.

For the theme customization, would using the default prop be enough?

Sorry, I didn't understand the question. My plan is to actually just use the LinearProgress with these extra stylings:

position: "fixed",
top: 0,
right: 0,
left: 0,
zIndex: 1201, // or perhaps `theme.zIndex.tooltip` ?

So does it also mean that I should clone LinearProgress theming or can we stick with LinearProgressProps? What do you think?

oliviertassinari commented 3 years ago

@italodeandra I hadn't considered the option to use the linear progress. I see two issues if we do such:

  1. Global customizations done to the LinearProgress will impact the NProgress component too. I fail to envision why customizations should be shared. For instance, it seems that what designers will most often want to tweak is the color of the NProgress. For instance, I was thinking of using a blue color, independent from the primary color of the theme.
  2. We pay the bundle size for props in the LinearProgress that are not relevant for the NProgress component.

Maybe I'm wrong?

italodeandra commented 3 years ago

I agree with you.

Does it make sense to copy the code from LinearProgress though? It is exactly what we need (the design, the progress animation, etc). Perhaps we should act like NProgress is a new component, but it uses LinearProgress internally. And we override the LinearProgress customization with NProgress's. And theme customization done to LinearProgress will also be ignored and keep NProgress default if no customization to the it's own theme is done.

What do you think?

oliviertassinari commented 3 years ago

@italodeandra I think that if the source is simple enough, a duplication could work great.

italodeandra commented 3 years ago

You're right. What worries me is maintenance, because they will need to have the same code if you want to keep it consistent. They will look the same but with some small differences, like the default color being secondary or contrastText instead of primary, being fixed to the top, and not having all the options LinearProgress has because we don't need all of them. So the designer might expect that they also have the same properties. But that is just my own assumption.

I'll duplicate the code, and remove what we're not going to use (like buffering). Then we can feel together if it could be a problem or just me overthinking.

italodeandra commented 3 years ago

@oliviertassinari Is it possible to use next/router on the demo iframe? I'm trying here without success. I'm adding an example for the Next.js usage while taking advantage that the doc app is actually Next.js. But perhaps it's not possible inside the iframe?

oliviertassinari commented 3 years ago

@italodeandra Regarding Next.js, I would recommend the following:

A side note on demo in iframe, there is a "iframe": true option you can leverage.

italodeandra commented 3 years ago

@oliviertassinari Thanks for answering.

  • Replace the one we use in the documentation with this new one

  • Describe how it can be done in the documentation with a code snippet

✅, I also added a demo with a basic implementation (with two buttons one to start and another to end it).

  • No demos in an iframe in the docs. It sounds really costly to put in place, with very little value.

Agree.

A side note on demo in iframe, there is a "iframe": true option you can leverage.

After some research on the codebase I found it. Very straight forward by the way.

italodeandra commented 3 years ago

About translations, I only know Brazilian Portuguese besides English. Can I only add those two?

oliviertassinari commented 3 years ago

@italodeandra the source of truth of the transition is in https://translate.material-ui.com. Your changes will be lost. I wouldn't encourage to care, for now.

bryanltobing commented 2 years ago

just want to clarify based on this closed PR https://github.com/mui/material-ui/pull/25889

so this component is currently not available yet in mui?

italodeandra commented 2 years ago

Hi @bryantobing12,

Yes, the NProgress component is not available on MUI. I was the initial responsible for it but its development was frozen.

I'm currently not working with MUI so I can't really invest time on it. If you're interested, you can continue it.