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 loading experience #20005

Merged
merged 5 commits into from
Mar 20, 2020
Merged

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Mar 5, 2020

The idea is the let react yield earlier. The components that are not pre-rendered don't have to be rendered synchronously because the user will notice the shift anyway. But we can increase TTI by defering them with a passive effect.

Note to self: While we do implement a fallback for NoSsr and have a component dedicated to fallback UIs in our library we do not even use these ourselves.

@eps1lon eps1lon added docs Improvements or additions to the documentation performance labels Mar 5, 2020
@mui-pr-bot
Copy link

mui-pr-bot commented Mar 5, 2020

No bundle size changes comparing 8d12012...161b736

Generated by 🚫 dangerJS against 161b736

@eps1lon eps1lon added the on hold There is a blocker, we need to wait label Mar 6, 2020
@oliviertassinari
Copy link
Member

I have used this trick in the past to speed up the page transition speed. It sounds promising. I'm not sure we will measure a difference on the initial load though (I would expect more an impact in client side navigation).

@eps1lon eps1lon removed the on hold There is a blocker, we need to wait label Mar 9, 2020
@mbrookes
Copy link
Member

Not sure about the loading indicator for the search textfield, as it isn't visible for long enough to see that that's what it is. Perhaps it could be a div styled the same as the input?

@eps1lon
Copy link
Member Author

eps1lon commented Mar 11, 2020

as it isn't visible for long enough to see that that's what it is.

Yeah the idea is more to have some shape in place that outlines the actual import. At some point NoSsr will become React.Suspense anyway and then we don't have to care about the displayed duration being too short.

I think the div suggestion is indicative of shortcomings of the Skeleton. Maybe add color variants that have better contrast e.g. <Skeleton on="primary" />?

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Removed the fallback from the server-rendered output. Suspense will handle the heavy lifting regarding display duration.

Following the latest version of the React's guide on suspense, it seems that delaying is not something they support natively nor they plan to? That it will be up to UI libraries to handle it? https://reactjs.org/docs/concurrent-mode-patterns.html#delaying-a-pending-indicator.

I have tried the following diff, it seems to provide a great DX (+ the diff on the toolbar I haven't shared) 😍.

diff --git a/docs/src/modules/components/AppFrame.js b/docs/src/modules/components/AppFrame.js
index f6ccd0608..550d13ca3 100644
--- a/docs/src/modules/components/AppFrame.js
+++ b/docs/src/modules/components/AppFrame.js
@@ -51,10 +51,29 @@ Router.onRouteChangeError = () => {
   NProgress.done();
 };

+const Delay = withStyles(theme => ({
+  root: {
+    opacity: 0,
+    animation: `${theme.transitions.duration.shorter}ms linear 0s forwards $makeVisible`,
+  },
+  '@keyframes makeVisible': {
+    '100%': {
+      opacity: 1,
+    },
+  },
+}))(({ timeout = 500, classes, ...other }) => (
+  <div className={classes.root} style={{ animationDelay: `${timeout}ms` }} {...other} />
+));
+
 const AppSearch = React.lazy(() => import('docs/src/modules/components/AppSearch'));
 function DeferredAppSearch(props) {
   const { className } = props;

+  const fallback = (
+    <Delay timeout={1000}>
+      <Skeleton className={className} height={40} variant="rect" width={200} />
+    </Delay>
+  );
   const [mounted, setMounted] = React.useState(false);
   React.useEffect(() => {
     setMounted(true);
@@ -69,12 +88,12 @@ function DeferredAppSearch(props) {
       />
       {/* Suspense isn't supported for SSR yet */}
       {mounted ? (
-        <React.Suspense
-          fallback={<Skeleton className={className} height={40} variant="rect" width={200} />}
-        >
+        <React.Suspense fallback={fallback}>
           <AppSearch className={className} />
         </React.Suspense>
-      ) : null}
+      ) : (
+        fallback
+      )}
     </React.Fragment>
   );
 }

I think that we can keep exploring the problem, I suspect we can release a new module to address it. We have a prior work on the delaying problem in https://material-ui.com/components/progress/#delaying-appearance. However, I have noticed two issues with the Fade component:

  1. It doesn't work server-side. In our case, it's important, we can notice a flash of skeleton for the demo's toolbar.
  2. It doesn't work with the Skeleton as it already leverages the opacity prop.

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 19, 2020

@eps1lon I can see two paths to move forward here.

  1. We remove the Skeleton from the demo's toolbar, I think that no flash (no distraction) is better than a skeleton, especially for a part of the UI that isn't required during the first seconds of rendering. I think that it's only once the developer has identified the demo he needs, that he might be interested in using the actions in the toolbar.
  2. We push this idea of a Delay component forward, leveraging the documentation and integration with the Skeleton as a practical use case

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 19, 2020

Regarding, 1. I'm tempted to say the same thing for the search bar. The the case of the toolbar, we can only see two states, it's skeleton or toolbar. However, in the case of the search bar, we can notice 3 states: empty, skeleton, searchbar. In a fast computer, it twinkles. Making a stronger distraction (away from the content of the page) than the toolbar.

@eps1lon
Copy link
Member Author

eps1lon commented Mar 19, 2020

I think that no flash (no distraction) is better than a skeleton

So we should remove the Skeleton from the core since it's a bad component in your opinion?

especially for a part of the UI that isn't required during the first seconds of rendering

Which is any lazy loaded part. That is the whole point of lazy loading: prioritizing required parts while deferring optional parts. You're essentially saying that skeletons are useless because they should only be used for required UI elements which means your whole UI shouldn't be rendered since it's missing required parts.

Making a stronger distraction (away from the content of the page) than the toolbar.

A skeleton isn't distracting. That's the whole point of it. To prepare content while the user can scan different parts of the UI

We push this idea of a Delay component forward, leveraging the documentation and integration with the Skeleton as a practical use case

This would be a step back. We want to get rid of fixed loading indicators and let Suspense handle these.

@eps1lon
Copy link
Member Author

eps1lon commented Mar 19, 2020

Also rejecting this whole "flashing" argument. This PR is objectively reducing "flash" by displaying an intermediate skeleton that is close to the final UI.

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 19, 2020

So we should remove the Skeleton from the core since it's a bad component in your opinion?

Without a defer logic, I can cause more harm than good, yes. At least, it's my personal experience comparing A with B. This is why I think that we should explore how to defer on the Material-UI side. It's actually awesome that we start to use it for our own documentation, it's the best way to improve these building blocks, by using them :D.

You're essentially saying that skeletons are useless because they should only be used for required UI elements which means your whole UI shouldn't be rendered since it's missing required parts.

I think that YouTube and Facebook are great benchmarks for what the Skeleton component is best used for: keep the attention of the users on the important elements of the page, create a spatial referential for when the important data will be loaded, help end-users process the information faster.

Also rejecting this whole "flashing" argument. This PR is objectively reducing "flash" by displaying an intermediate skeleton that is close to the final UI.

That's a really interesting perspective as mine is completely opposed 😄. I feel more mentally loaded with these skeletons in place. On the other hand, using the Skeleton for the body of the page, like Vuetify is doing feels fine: https://vuetifyjs.com/en/styles/text/ (visible on client-side navigations). I think that we need more feedback cc @joshwooding and @mbrookes

We want to get rid of fixed loading indicators and let Suspense handle these.

Do you have a resource showing that Suspense will? I can no longer find a reference to a timeout option (outside useTransition that doesn't do what we look for) on this unstable feature.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 20, 2020
@eps1lon
Copy link
Member Author

eps1lon commented Mar 20, 2020

good bot :)

@eps1lon eps1lon closed this Mar 20, 2020
@eps1lon eps1lon reopened this Mar 20, 2020
@eps1lon
Copy link
Member Author

eps1lon commented Mar 20, 2020

Removed skeletons

Note to self: While we do implement a fallback for NoSsr and have a component dedicated to fallback UIs in our library we do not even use these ourselves.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 20, 2020
@oliviertassinari oliviertassinari merged commit f008465 into mui:master Mar 20, 2020
@oliviertassinari
Copy link
Member

@eps1lon Thank you!

@eps1lon eps1lon deleted the docs/lazy-nossr branch March 24, 2020 18:48
EsoterikStare pushed a commit to EsoterikStare/material-ui that referenced this pull request Mar 30, 2020
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 performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants