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

Infinite scroll for all views #2859

Merged
merged 17 commits into from
Apr 29, 2022

Conversation

Jarsen136
Copy link
Contributor

@Jarsen136 Jarsen136 commented Apr 15, 2022

Thank you for your contribution to the KodaDot NFT gallery.
👇 _ Let's make a quick check before the contribution.

PR type

  • Bugfix
  • Feature
  • Refactoring

What's new?

Process:

  • explore-collection infinite scroll on collection page #2781
  • explore-gallery
  • collection/{id}
  • profile-created
  • profile-collection
    test case: /rmrk/u/FZpjhYdt6fqRSzW1nR4fRejavcrSo7UXYP4TDhE3mgimgeC
  • profile-sold
  • profile-collected

Before submitting Pull Request, please make sure:

  • My contribution builds clean without any errors or warnings
  • I've merged recent default branch -- main and I've no conflicts
  • I've tried to respect high code quality standards
  • I've didn't break any original functionality
  • I've posted a screenshot of demonstrated change in this PR

Optional

  • I've tested it at </rmrk/collection/26902bc2f7c20c546a-1FVG7>
  • I've tested PR on mobile and everything seems works
  • I found edge cases
  • I've written some unit tests 🧪

Had issue bounty label?

  • Fill up your KSM address: Payout

Community participation

Screenshot

  • My fix has changed something on UI; a screenshot is best to understand changes for others.

@Jarsen136 Jarsen136 requested a review from a team as a code owner April 15, 2022 11:08
@Jarsen136 Jarsen136 requested review from prachi00 and removed request for a team April 15, 2022 11:08
@netlify
Copy link

netlify bot commented Apr 15, 2022

Deploy Preview for koda-nuxt ready!

Name Link
🔨 Latest commit b350763
🔍 Latest deploy log https://app.netlify.com/sites/koda-nuxt/deploys/626a54db2f225300081ed88c
😎 Deploy Preview https://deploy-preview-2859--koda-nuxt.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 settings.

@Jarsen136 Jarsen136 changed the title infinite scroll for all views WIP: infinite scroll for all views Apr 15, 2022
@Jarsen136
Copy link
Contributor Author

Infinite scroll has been done in collection/{id}. @roiLeo
Feel free to leave some comments for code review if you have

Copy link
Contributor

@roiLeo roiLeo left a comment

Choose a reason for hiding this comment

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

  • why do you need a callback in async fetchPageData?
  • it is necessary to have a loadDirection?
  • can we try to use fetch hook?
  • would be cool to add in a near future when there is "No Data" or the "End of Data", I think vue-infinite-slot provide slot template

@Jarsen136
Copy link
Contributor Author

  • why do you need a callback in async fetchPageData?

Because it needs to notify the fetching status changed.
The relevant code is in infiniteScrollMinxin.ts

  • it is necessary to have a loadDirection?

Yes, it is necessary to know in which direction will the paging data inserted.
For example, you already have a list and then you fetch paging data.
With the direction, you can directly know whether to insert it at the front or the back of the list

Sorry but I still don't know the benefit if use fetch hook.
A custom fetch function fetchPageData may be more flexible?

  • would be cool to add in a near future when there is "No Data" or the "End of Data", I think vue-infinite-slot provide slot template

Yes, but let's finish the infinite scroll in all views first because there is still a lot of work to be done : )
The display of "No Data" and the "End of Data" needs some Ui redesign.

@Jarsen136 Jarsen136 changed the title WIP: infinite scroll for all views Infinite scroll for all views Apr 16, 2022
@Jarsen136 Jarsen136 requested a review from roiLeo April 16, 2022 07:03
@yangwao
Copy link
Member

yangwao commented Apr 16, 2022

Probably go on https://deploy-preview-2859--koda-nuxt.netlify.app/rmrk/explore?page=1 collections with small tiles as default?
🐛 - filters seem not in one row

image

@Jarsen136
Copy link
Contributor Author

Probably go on https://deploy-preview-2859--koda-nuxt.netlify.app/rmrk/explore?page=1 collections with small tiles as default? 🐛 - filters seem not in one row

image

will check it later.

@Jarsen136
Copy link
Contributor Author

Probably go on https://deploy-preview-2859--koda-nuxt.netlify.app/rmrk/explore?page=1 collections with small tiles as default? 🐛 - filters seem not in one row

  • 🐛Fixed - filters seem not in one row

image

@Jarsen136
Copy link
Contributor Author

Let's merge #2871 first, and then I could apply this fix to the other views and resolve conflicting code.

@roiLeo roiLeo marked this pull request as draft April 20, 2022 08:30
@Jarsen136 Jarsen136 marked this pull request as ready for review April 20, 2022 15:44
@Jarsen136
Copy link
Contributor Author

Let's merge #2871 first, and then I could apply this fix to the other views and resolve conflicting code.

Already modified code. Ready for code review now.

@Jarsen136 Jarsen136 requested a review from vikiival April 26, 2022 14:18
first: this.first,
offset: this.offset,
}
variables: {
Copy link
Member

Choose a reason for hiding this comment

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

Why u changed that from function to object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because $apollo.query need variables to be an object.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@Jarsen136 Jarsen136 Apr 28, 2022

Choose a reason for hiding this comment

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

Both function and object could work.

However, the query does not need to be 'reactive' in this case.
So I use the object to be the param. 🤔

And if you have other reasons to use 'function', I could change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed my mind after doing some tries, because using function as variables could not work.

$apollo.query only accept 'object' in this case.

image

@yangwao
Copy link
Member

yangwao commented Apr 27, 2022

I manage to break it?
Scrolled somewhere down to the higher page and then went slowly back.
When I'm on top of the collection my bet page would be around the first page?

https://deploy-preview-2859--koda-nuxt.netlify.app/rmrk/collection/3892e43e923e5ad973-KAG?page=76
image

Another bug if I tried open this, did not load anything https://deploy-preview-2859--koda-nuxt.netlify.app/rmrk/collection/3892e43e923e5ad973-KAG?page=395

image

@yangwao
Copy link
Member

yangwao commented Apr 27, 2022

How is paging made? Because I scroll like 1-5 pixels vertically and change pages number pretty wild 😁

Screen.Recording.2022-04-27.at.14.16.04.mov

@yangwao yangwao mentioned this pull request Apr 27, 2022
20 tasks
@Jarsen136
Copy link
Contributor Author

oh, I will check it later.

@Jarsen136
Copy link
Contributor Author

It is because the collection has only 26 pages of items, however the page number on the url is greater than this maximum.
I have done something to avoid it.

Let's see if your issue has been resolved now? @yangwao

@Jarsen136 Jarsen136 requested a review from vikiival April 28, 2022 12:45
@yangwao
Copy link
Member

yangwao commented Apr 29, 2022

Seem the previous stuff has been fixed.

Follow up issue could be

What I see when open in explorer
image
What it supposed to be
image

@yangwao
Copy link
Member

yangwao commented Apr 29, 2022

pay 300 usd

@yangwao
Copy link
Member

yangwao commented Apr 29, 2022

😍 Perfect, I’ve sent the payout
💵 $300 @ 155.56 USD/KSM ~ 1.929 $KSM
🧗 Caiv9TbPz68q5dC8EcHu5xKYPRnremimGzqmEejDFNpWWLG
🔗 0x474ea6f876753de0207db5ec1b2db8e399861de63154a148e89e06467d9b09ba

🪅 Let’s grab another issue and get rewarded!
🪄 github.com/kodadot/nft-gallery/issues

@yangwao yangwao added the paid pull-request has been paid label Apr 29, 2022
@yangwao yangwao merged commit 8379cd9 into kodadot:main Apr 29, 2022
@Jarsen136 Jarsen136 mentioned this pull request May 1, 2022
17 tasks
@roiLeo roiLeo mentioned this pull request May 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
paid pull-request has been paid
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Infinite scrolling on demand
4 participants