-
-
Notifications
You must be signed in to change notification settings - Fork 347
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
refactor: Infinitescroll Vue3 composition api #5108
Conversation
SUCCESS @Jarsen136 PR for issue #4952 which is assigned to you. Please wait for review and don't hesitate to grab another issue in the meantime! |
✅ Deploy Preview for koda-nuxt ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
composables/useInfiniteScroll.ts
Outdated
|
||
type LoadDirection = 'up' | 'down' | ||
|
||
export default function ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function default
has a Cognitive Complexity of 29 (exceeds 5 allowed). Consider refactoring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are my concern:
useDebounceFn
😐- 200+ LOC
- DOM elements access (no ssr)
- event listener
have you tried to use vInfiniteScroll
from @vueuse/components?
It's necessary to use
I'm really curious about how could we improve the ssr in this case. I could not think of anything related to it TBH. Could you pls expand more?
We have to listen to the scroll event to know which page we are visiting.
I have read the docs about useInfiniteScroll. The function provided could not fit our requirements. It could only provide single-direction scrolling. In our codebase, the infinite scroll mixins provide much more functionality, like a double-direction scroll, prefetch data, pagination etc. That's why it has so much LOC :) |
hmmm, okay
maybe use
I'll take a look when I have time to make it work with vueuse. pretty sure it's doable |
Up direction has up button anyway so I would check if user on first hit is on other page than one. |
Sorry, I just thought ssr is server-side render. I guess use ref does not change anything to it. Let me know if I miss something.
However, it binds the scroll event on
emm. Maybe use twice time to bind two directions. I'm not sure if would work as expected. Anyway, I'm willing to see if someone could improve the code. |
The up button for loading the previous is only for the explore gallery page because it has a special structure. I wonder if we would like to apply the up button to other pages also. If so, I could try to do some refactoring and use [useInfiniteScroll] for the down-direction loading. double-direction scroll example: https://beta.kodadot.xyz/rmrk/explore/collectibles?page=3 |
hey haven't read anything on this pr, just having head's up on this that we should probably drop infinity scroll to the top and replace it to the button best which adds 4 rows with extra items |
OKi, I will make it work. |
I have created a Here is the preview path: https://deploy-preview-5108--koda-nuxt.netlify.app/rmrk/explore/collectibles |
seems working well, can try to add to items? |
Yes, I will refactor GalleryList component to composition api first, and then I would apply the infinitescroll to items. |
✅ added infiniteScroll to redesign items page preview: https://deploy-preview-5108--koda-nuxt.netlify.app/bsx/explore/items?redesign=true |
<a | ||
v-if="startPage > 1 && !isLoading && total > 0" | ||
class="is-flex is-justify-content-center pb-4" | ||
@click="reachTopHandler"> | ||
<b-icon icon="chevron-up" /> | ||
</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just one thing, I would make this as component because we plan to reuse that across app
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DONE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let it break 👀
Code Climate has analyzed commit 6cffd72 and detected 3 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
pay 50 usd |
😍 Perfect, I’ve sent the payout 🪅 Let’s grab another issue and get rewarded! |
Thank you for your contribution to the KodaDot NFT gallery.
👇 _ Let's make a quick check before the contribution.
PR Type
Context
Before submitting pull request, please make sure:
Optional
Had issue bounty label?
Community participation
Screenshot 📸