search

apal21 / nextjs-progressbar

A simple Next.js progressbar component using NProgress.
https://www.npmjs.com/package/nextjs-progressbar
MIT License
780 stars 61 forks source link

Fix incorrect types in readme #11

Closed lil-j closed 3 years ago

lil-j commented 3 years ago

startPosition should be a number, not a string.

apal21 commented 3 years ago

Hey, thanks for the PR.

Just wanted to know one thing: Is this causing any issue if we use string? Haven't checked yet.

lil-j commented 3 years ago

It still works, but by putting a number instead of a string the progress loads more accurately to the requests and progress of the load, overall much smoother IMO. Definitely give it a try and let me know if it does the same for you 😄

apal21 commented 3 years ago

Okay, I checked the code of Nprogress. The string would work just fine without any problem because of these lines: https://github.com/rstacruz/nprogress/blob/master/nprogress.js#L325-L326

In Javascript, we can compare a number of type string (eg. "123") with a normal number (123). That means "0.3" < 1 // returns true and "0.3" > 1 // returns false.

So this cannot have any performance issues. Still, I'm not closing this PR. Let me know if you find anything else which I couldn't think of or it can cause any problems. We can discuss it. 😇

lil-j commented 3 years ago

Hi. This may be true but I have created two websites to illustrate what I am talking about. Both are identical except one uses a string, and the other uses a number. Notice how the one with a string does not show the progress bar until after the page is loaded while the one with a number begins as the request begins, showing a more accurate and true line of progress. With String With Number

lil-j commented 3 years ago

Here is the code if you are interested.

apal21 commented 3 years ago

Insightful!!! Give me some time. I want to do some research on this. I'll get back to this soon. Thanks for creating this!

lil-j commented 3 years ago

No problem man. I just fixed some typos on the site to make some of the wording more clear as well. Keep me posted on your findings!

lil-j commented 3 years ago

Hey any updates?

apal21 commented 3 years ago

Apologies... Last time I couldn't get time. I'll be working to resolve this on this weekend. Let me know if you find anything.

apal21 commented 3 years ago

Hey @lil-j, I saw the same thing happening with stopDelayMs. Can you please change that also and update the title of this PR? I'll merge it.


<NextNprogress
  color="#29D"
  startPosition={0.3}
  stopDelayMs={200}
  height="3"
/>