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

add skeleton when query is loading #5872

Merged
merged 14 commits into from
May 16, 2023
Merged

add skeleton when query is loading #5872

merged 14 commits into from
May 16, 2023

Conversation

leo-anderson-x
Copy link
Contributor

@leo-anderson-x leo-anderson-x commented May 6, 2023

Thank you for your contribution to the KodaDot - One Stop Shop for Polkadot NFTs.

👇 __ Let's make a quick check before the contribution.

PR Type

  • Feature

Context

Had issue bounty label?

  • Fill up your KSM address: Payout

Screenshot 📸

  • My fix has changed something on UI; a screenshot is best to understand changes for others.
CleanShot.2023-05-06.at.20.55.03.mp4

Copilot Summary

🤖 Generated by Copilot at 93ba4fe

This pull request adds loading states for the collection list and items grid components, using skeleton components to show placeholders while the data is loading. It also adds two new skeleton components, CollectionCardSkeleton.vue and NeoNftCardSkeleton.vue, to the @kodadot1/brick library, and exports them for reuse.

🤖 Generated by Copilot at 93ba4fe

Skeletons appear
b-skeleton shapes the cards
Winter of loading

@leo-anderson-x leo-anderson-x requested a review from a team as a code owner May 6, 2023 11:56
@leo-anderson-x leo-anderson-x requested review from roiLeo and Jarsen136 and removed request for a team May 6, 2023 11:56
@kodabot
Copy link
Collaborator

kodabot commented May 6, 2023

SUCCESS @leo-anderson-x PR for issue #5749 which is assigned to you. Please wait for review and don't hesitate to grab another issue in the meantime!

@netlify
Copy link

netlify bot commented May 6, 2023

Deploy Preview for koda-canary ready!

Name Link
🔨 Latest commit 97118dc
🔍 Latest deploy log https://app.netlify.com/sites/koda-canary/deploys/64621bf84a911700086c7bfc
😎 Deploy Preview https://deploy-preview-5872--koda-canary.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.

@reviewpad
Copy link
Contributor

reviewpad bot commented May 6, 2023

AI-Generated Summary: This pull request adds skeleton loading indicators for query results in the CollectionList and ItemsGrid components. The implementation includes the creation of CollectionCardSkeleton and NeoNftCardSkeleton components, as well as modifications to the CollectionList, ItemsGrid, and index files.

@reviewpad reviewpad bot added medium Pull request is medium waiting-for-review labels May 6, 2023
@exezbcz
Copy link
Member

exezbcz commented May 7, 2023

nice! works for me 🚀

wdyt about removing hover on skeleton load? Does that make sense or nah?

@leo-anderson-x
Copy link
Contributor Author

leo-anderson-x commented May 8, 2023

removing hover on skeleton load?

@exezbcz good idea, updated

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.

Coule you please use NeoSkeleton component instead of b-skeleton ?

@yangwao yangwao added the S-changes-requested-🤞 PR is almost good to go, just some fine tunning label May 8, 2023
@leo-anderson-x
Copy link
Contributor Author

leo-anderson-x commented May 9, 2023

Coule you please use NeoSkeleton component instead of b-skeleton ?

@roiLeo updated

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.

Doesn't look perfect on large screen, maybe increase numbers: 12 for collections and 14 for items
Capture d’écran 2023-05-09 à 4 10 36 PM

Capture d’écran 2023-05-09 à 4 18 53 PM

FYI: no-margin = no-margin="true"

components/collection/CollectionCard.vue Outdated Show resolved Hide resolved
components/collection/CollectionDetail.vue Outdated Show resolved Hide resolved
components/collection/CollectionList.vue Outdated Show resolved Hide resolved
libs/ui/src/components/NeoNftCard/NeoNftCard.vue Outdated Show resolved Hide resolved
@prury prury added the S-works-for-me-✅ qa-guild has tested PR from end user perspective and functionality worked label May 9, 2023
Copy link
Member

@vikiival vikiival left a comment

Choose a reason for hiding this comment

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

Pointed out by @roiLeo and @Jarsen136

@leo-anderson-x
Copy link
Contributor Author

Doesn't look perfect on large screen, maybe increase numbers: 12 for collections and 14 for items

@roiLeo updated to: if screen width < 1024 (at this point 4 is enough), show 4, else show 12 collections and 14 items.

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.

I find it more readable to put skeleton near <DynamicGrid /> component, so you'll have all 3 options in the same file (data, loading skeleton & empty data)
Did you try to put it on that level?

@leo-anderson-x
Copy link
Contributor Author

leo-anderson-x commented May 11, 2023

I find it more readable to put skeleton near <DynamicGrid /> component, so you'll have all 3 options in the same file (data, loading skeleton & empty data) Did you try to put it on that level?

@roiLeo u means the following three options in parallel, right?

<DynamicGrid v-if="total !== 0">...</DynamicGrid>

<template v-else-if="isLoading">
  <CollectionCard v-for="n in SKELETON_COUNT" :key="n" is-loading />
</template>

<EmptyResult v-else />

But the skeletons also needs to be wrapped in a DynamicGrid to get the same size as a normal Card (it is dynamically derived from the screen size). So if we place three options in parallel, the result will look like this:

<DynamicGrid v-if="total !== 0">...</DynamicGrid>

<DynamicGrid v-else-if="isLoading" {...same props as above}>
  <CollectionCard v-for="n in SKELETON_COUNT" :key="n" is-loading />
</DynamicGrid>

<EmptyResult v-else />

This seemed a bit repetitive, so I chose to put the skeletons inside the first <DynamicGrid />, rather than alongside the first <DynamicGrid /> like the <EmptyResult />.

@roiLeo
Copy link
Contributor

roiLeo commented May 11, 2023

I find it more readable to put skeleton near <DynamicGrid /> component, so you'll have all 3 options in the same file (data, loading skeleton & empty data) Did you try to put it on that level?

@roiLeo u means the following three options in parallel, right?

<DynamicGrid>...</DynamicGrid>

<template v-if="isLoading">
  <CollectionCard v-for="n in SKELETON_COUNT" :key="n" is-loading />
</template>

<EmptyResult v-if="total === 0 && !isLoading" />

Yes I find something like that easier to maintain

But the skeletons also needs to be wrapped in a DynamicGrid to get the same size as a normal Card (it is dynamically derived from the screen size). So if we place three options in parallel, the result will look like this:

<DynamicGrid>...</DynamicGrid>

<DynamicGrid v-if="isLoading" {...same props as above}>
  <CollectionCard v-for="n in SKELETON_COUNT" :key="n" is-loading />
</DynamicGrid>

<template v-if="isLoading">
  <CollectionCard v-for="n in SKELETON_COUNT" :key="n" is-loading />
</template>

<EmptyResult v-if="total === 0 && !isLoading" />

Hmmmm... not sure I'm following you're idea, more like:

<DynamicGrid v-if="!isLoading && total">...</DynamicGrid> 

<template v-if="isLoading">
  <!-- use same limit as query -->
  <div v-for="index in limit" :key="index">
    <div class="collection-card card">
        (Here you manage to recreate simple card using same class with skeleton components)
    </div>
  </div>
</template>
 
<EmptyResult v-if="total === 0 && !isLoading" />

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.

✅ works for me anyway

components/collection/CollectionList.vue Outdated Show resolved Hide resolved
@vikiival
Copy link
Member

Please resolve conflicts +
@roiLeo has some comments

# Conflicts:
#	components/items/ItemsGrid/ItemsGrid.vue
#	libs/ui/src/components/NeoNftCard/NeoNftCard.vue
@prury
Copy link
Member

prury commented May 12, 2023

will check again when it's ready

@vikiival
Copy link
Member

@prury should be ready

@codeclimate
Copy link

codeclimate bot commented May 15, 2023

Code Climate has analyzed commit 97118dc and detected 0 issues on this pull request.

View more on Code Climate.

@Jarsen136 Jarsen136 removed the S-changes-requested-🤞 PR is almost good to go, just some fine tunning label May 15, 2023
@prury
Copy link
Member

prury commented May 15, 2023

@prury should be ready

Its showing 40 skeletons for both collections and items for me, if that is intended, works well, had to use a bandwidth limiter to test it.

  • tested on android, firefox, chrome.
  • responsiveness looking good

@yangwao
Copy link
Member

yangwao commented May 16, 2023

pay 30 usd

@yangwao
Copy link
Member

yangwao commented May 16, 2023

😍 Perfect, I’ve sent the payout
💵 $30 @ 25.57 USD/KSM ~ 1.173 $KSM
🧗 DzUbHCk3Fr3XdCPNKo7uCJvtBH7YfgiFbn4Gr3VmCMiFy1C
🔗 0x754109af7f1c6e183f3a7c5bd2f6d6358d47b2e885c30866404cdb17cd6d37f9

🪅 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 May 16, 2023
@yangwao
Copy link
Member

yangwao commented May 16, 2023

@yangwao yangwao merged commit 930c5a0 into kodadot:main May 16, 2023
16 of 19 checks passed
@leo-anderson-x leo-anderson-x deleted the 5749 branch May 16, 2023 11:23
@leo-anderson-x
Copy link
Contributor Author

Its showing 40 skeletons for both collections and items for me, if that is intended, works well

@prury Yes, it's intended. We use same size as fetched data

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
medium Pull request is medium paid pull-request has been paid S-visual-ok-✅ S-works-for-me-✅ qa-guild has tested PR from end user perspective and functionality worked waiting-for-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default loading dialog on explorer and collections
8 participants