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

[gatsby-plugin-nprogress]: change default show delay #3368

Closed
peduarte opened this issue Dec 30, 2017 · 17 comments
Closed

[gatsby-plugin-nprogress]: change default show delay #3368

peduarte opened this issue Dec 30, 2017 · 17 comments

Comments

@peduarte
Copy link
Contributor

Hello,

I've just implemented gatsby-plugin-nprogress. Easy to set up and works out of the box, love it.

However I was wondering if it's possible to change the default show delay. What I mean is by default Gatsby displays the NProgress component after 1 second of a link being clicked - I'd like to change this to something shorter, perhaps 600ms.

Thanks a lot

@peduarte
Copy link
Contributor Author

peduarte commented Jan 8, 2018

After taking a look at the source code, I may have found the piece of code which hardcodes the 1000 ms timeout:

// Start a timer to wait for a second before transitioning and showing a
// loader in case resources aren't around yet.
const timeoutId = setTimeout(() => {
emitter.off(`onPostLoadPageResources`, eventHandler)
emitter.emit(`onDelayedLoadPageResources`, { pathname })
window.___history.push(pathname)
}, 1000)

And here's the plugin's code which listens to the onDelayedLoadPageResources event:

window.___emitter.on(`onDelayedLoadPageResources`, () => {
NProgress.configure(pluginOptions)
NProgress.start()
})

If this was to be configurable, how would you go about doing so?

Happy to contribute with some guidance.

Thanks 👍

@KyleAMathews
Copy link
Contributor

Any particular reason why 600ms vs. 1000ms? ~1000ms is the well-tested point at which people start to think something is wrong if there's not feedback https://www.nngroup.com/articles/response-times-3-important-limits/ which is why that's the value that's hard-coded.

@peduarte
Copy link
Contributor Author

I've been looking for an article like this, plus I love the research made by NN/g. Thanks for this 👍

However while testing my website I had the feeling that 1000ms with no feedback was a bit too long, probably because I fade in the content, making it seem even slower? I can take a look and see if I can improve it on my side.

Looking at websites such as Medium and YouTube I have the feeling that the Page Loading Bars are displayed sooner than 1000ms. Additionally Pace suggests ~500ms.

It seems to me that although 1000ms is a good rule of thumb, it's also subjective, which makes me question if Gatsby should allow the user to set a custom onDelayedLoadPageResources value.

Would be happy to hear about what other users think of this too 🙂

@KyleAMathews
Copy link
Contributor

Subjective???? This is SCIENCE!!! :-P

Fair points :-) I'd be up for a PR that lets you override the default. Probably should be done via gatsby-browser.js so plugins can use it. Perhaps replaceDelayedPageLoadConstant or something like that https://www.gatsbyjs.org/docs/browser-apis/

gatsby-plugin-nprogress could then take an option for changing the timeout.

We should also make a proper API for calling plugins when the page load is delayed instead of using the internal emitter.

@peduarte
Copy link
Contributor Author

@KyleAMathews Cool, I'll take a look then! 🙂

I agree that it feels weird to use the internal event for plugins, but should this refactor be done as a separate PR?

@KyleAMathews
Copy link
Contributor

Might as well do it at the same time.

@peduarte
Copy link
Contributor Author

@KyleAMathews Ok, so I've forked and cloned the Gatsby repo. Bootstrap is done and test have passed.

As I've never worked on Gatsby before, I'm struggling to find my way around.

You mentioned the gatsby-plugin-nprogress could take an option for changing the timeout, however, as this is going to be emitted from a global onDelayedLoadPageResources event, the plugin's options doesn't seem like the right place for it (other plugins/components could potentially subscribe to this event too).

If this is a global setting, where should the user define this option?

Is there any sort of customisation like this in place?

I'll appreciate any guidance 🙂

Thanks!

@KyleAMathews
Copy link
Contributor

Sure!

There's a function in production-app.js named apiRunner which lets you run functions provided by plugins e.g.

const results = apiRunner(`shouldUpdateScroll`, {

You'll want to create a new API called onPageResourceLoadingDelayed and document it at https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby/src/utils/api-browser-docs.js

Use that to replace the current event emitter stuff https://github.com/gatsbyjs/gatsby/search?utf8=%E2%9C%93&q=onDelayedLoadPageResources&type=

Then for the delay time, add another API called replaceDelayedPageLoadConstant. Call that before starting the timer and check if any plugin returns a value and use the last numeric value that's return or if no value is returned use 1000 — this is similar to

if (results.length > 0) {

@busticated
Copy link
Contributor

good stuff ya'll 👏👍

just adding my $.02 - hopefully not a stick in the spokes :)

it would be awesome if we had an event / hook / whatever that let us show something at the start of navigation. today we have the notions of "delayed" and "done" but not "started" (unless i'm derping 🤪)

@peduarte
Copy link
Contributor Author

peduarte commented Jan 11, 2018

@busticated I agree... It's like a navigation lifecycle API.

Some ideas below:

  • onPageLoadStarted
  • onPageLoadEnded
  • onPageLoadDelayed

These events could then be used by plugins.

It may be better to have a separate task to uniform all the internal event naming convention too as well as introducing new ones - so PRs are easier to review.

@KyleAMathews
Copy link
Contributor

Yeah 💯 to creating standard lifecycle APIs

@peduarte
Copy link
Contributor Author

peduarte commented Jan 11, 2018

@KyleAMathews
I’m running my site via my forked clone of Gatsby and gatsby-dev however the following events in the nprogress plugin code never seem to get called:

  • onDelayedPageResources
  • onPostLoadPageResources

Is this intentional? (Perhaps because they’re fired from production-app.js and this doesn’t get called in development mode?)

Sorry for the stupid questions 😳

@KyleAMathews
Copy link
Contributor

Not a stupid question :-)

First yes, they'll only show up in "production" i.e. you need to build the site and then load it with gatsby serve

Second, those APIs don't exist yet :-) So you'll also need to add those by calling apiRunner

@peduarte
Copy link
Contributor Author

@KyleAMathews Ah, that explains it.

Ok, I've been having some thoughts about how this work could be done in smaller PRs. This is my first contribution to Gatsby and I think as an initial PR it's a bit overwhelming to start refactoring internal events and creating a navigation lifecycle API.

I'd like to propose the following:

  • A PR for defining a custom delay value until NProgress is displayed
  • A PR for refactoring Navigation Lifecycle API

Does that sound reasonable for you?

@KyleAMathews
Copy link
Contributor

Sure!

@peduarte
Copy link
Contributor Author

Closing this in favour of #3528 and #3530

@VickyPhoton
Copy link

So is it possible to reduce the delay from 1000 to 0 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants