acekyd / made-in-nigeria

Here is a curation of awesome tools built by Nigerians that can be used by anybody and from anywhere in the world.
https://madeinnigeria.dev
1.01k stars 401 forks source link

chore: performance improvements, infinite scrolling. #282

Closed kaf-lamed-beyt closed 8 months ago

kaf-lamed-beyt commented 8 months ago

@acekyd, kindly review when you see this.

codesandbox[bot] commented 8 months ago

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders
Open Preview

netlify[bot] commented 8 months ago

Deploy Preview for staging-madeinnigeria ready!

Name Link
Latest commit 2f6d1f7aeba7a07b144717fdc91b0c096efe1548
Latest deploy log https://app.netlify.com/sites/staging-madeinnigeria/deploys/65ca4f2fa1570d00084b2e27
Deploy Preview https://deploy-preview-282--staging-madeinnigeria.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

acekyd commented 8 months ago

Hey @kaf-lamed-beyt. Thanks for the PR.

In the mean time, can we revert the infinite loading / project search updates so we can have that in a different PR to figure it out properly.

What do you think?

kaf-lamed-beyt commented 8 months ago

When you mean... "the page keeps resetting", you're referring to when it scrolls back to the top yes?

Project search is broken with this, the typed text disappears

Does this happen when the page scrolls back to the top too?

Only those three properties are essential - name, author and description - but I see the problem if it has to run the loop thrice on keypress. The current live implementation uses some form of angular subscription.

Yes, it'll impact the response time.

In the mean time, can we revert the infinite loading / project search updates so we can have that in a different PR to figure it out properly.

Let me see if I can figure out why the reset happens. Once I find a fix, I'll push. If not, I'll revert both of them, and open a new PR to address it.

acekyd commented 8 months ago

Yes it scrolls back to the top, sometimes new content is added, sometimes it goes back to before any content was added. Is that not the same experience for you?

The typed text for search disappears about a second after you stopped typing. And I also just noticed the projects page keeps refreshing after every couple seconds even if you're not doing anything.

kaf-lamed-beyt commented 8 months ago

Scrolling back to the top before new content is added is new to me. I noticed the former, earlier on.

The pages is refreshing now? That's weird. I'll have a look at it now and get back to you.

kaf-lamed-beyt commented 8 months ago

Figured out the reason why the "scroll-to-top" happens on the /creators route.

It is a default behaviour of button elements. event.preventDefault() should fix it — hopefully on the projects route too.

kaf-lamed-beyt commented 8 months ago

Hi @acekyd, please take a look now and let me know if the issues persist.

About the refreshes even when we're not doing anything. I did not notice anything like that. But, please let me know if it persists, nonetheless.

acekyd commented 8 months ago

Hey! Can you merge the current web-dev into this your branch? I want to test it in full but I'm on the move. @kaf-lamed-beyt

acekyd commented 8 months ago

Check out the deploy preview - page still reloading and hence issues still persist.

https://deploy-preview-282--staging-madeinnigeria.netlify.app/projects

kaf-lamed-beyt commented 8 months ago

Huh. that's weird. Let me check if my branch is updated.

I'll have a look at the deploy preview too.

kaf-lamed-beyt commented 8 months ago

Oops! I experienced the refresh.

My PC's battery is low now. When there's power. I'll inspect the deploy preview link — properly with chrome devtools

That aside, @acekyd, help me test it on localhost. I want to be sure the issue doesn't persist on localhost so I can know how to proceed.

kaf-lamed-beyt commented 8 months ago

@acekyd, apparently, omitting the next property in the fetch URL was the problem. Why I had it removed, I have no idea.

const res = await fetch(
 "https://raw.githubusercontent.com/acekyd/made-in-nigeria/master/README.MD",
 { next: { revalidate: 5000 } }
);
acekyd commented 8 months ago

Looking great! Thank you. A few things:

Thanks again @kaf-lamed-beyt

kaf-lamed-beyt commented 8 months ago

The load more articles button still goes back to top of page

That's weird. I'll take a look at it again.

The search experience feels less optimal because it's currently only searching the title

So, should we include other properties? Which one would you suggest — repoDescription, and repoAuthor?

Reduce the font (intensity in general) of the "You have reached the end / no repositories found / no more articles" messages. Noted. I'll have this changed.

acekyd commented 8 months ago

Yes I think just those two. @kaf-lamed-beyt

kaf-lamed-beyt commented 8 months ago

Yes I think just those two. @kaf-lamed-beyt

Super!

kaf-lamed-beyt commented 8 months ago

@acekyd, kindly take a look now and let me know if the issues are gone.

acekyd commented 8 months ago

Looks great. Thanks @kaf-lamed-beyt