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 related and visited NFTs #2722

Merged
merged 22 commits into from
Apr 2, 2022
Merged

Add related and visited NFTs #2722

merged 22 commits into from
Apr 2, 2022

Conversation

preschian
Copy link
Member

@preschian preschian commented Mar 31, 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?

Before submitting Pull Request, please make sure:

  • My contribution builds clean without any errors or warnings
  • I've merged the 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.

Screen Shot 2022-03-31 at 9 17 29 PM

@netlify
Copy link

netlify bot commented Mar 31, 2022

Deploy Preview for koda-nuxt ready!

Name Link
🔨 Latest commit e1e246c
🔍 Latest deploy log https://app.netlify.com/sites/koda-nuxt/deploys/6247350bc9f8d10009dd0fd7
😎 Deploy Preview https://deploy-preview-2722--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.

@preschian preschian marked this pull request as ready for review March 31, 2022 14:04
@preschian
Copy link
Member Author

oops, I forgot to mention the order. currently orderBy: createdAt_DESC


let me try with that

@preschian
Copy link
Member Author

Updated with sorted by cheapest, and then newest. orderBy: [price_ASC, updatedAt_DESC]. Is the graphql query correct?

Screen Shot 2022-03-31 at 11 01 29 PM

@yangwao yangwao self-requested a review March 31, 2022 16:35
@yangwao
Copy link
Member

yangwao commented Mar 31, 2022

seems to work well, will wait for code review

image

image

@yangwao yangwao requested a review from vikiival March 31, 2022 16:36
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.

Let's make the code more reusable 🥺

components/base/CarouselCardList.vue Outdated Show resolved Hide resolved
queries/rmrk/subsquid/collectionEntityById.graphql Outdated Show resolved Hide resolved
components/rmrk/Gallery/VisitedNFT.vue Outdated Show resolved Hide resolved
components/rmrk/Gallery/VisitedNFT.vue Outdated Show resolved Hide resolved
@vikiival
Copy link
Member

vikiival commented Apr 1, 2022

Also both new components are doing the same logic :) it would be far better to make one common component which can tackle both.

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.

🙈

components/rmrk/Gallery/GalleryItemRelated.vue Outdated Show resolved Hide resolved
@preschian
Copy link
Member Author

DeepScan scans unrelated files in this PR 🤔

Screen Shot 2022-04-01 at 6 07 39 PM

components/rmrk/Gallery/GalleryItemRelated.vue Outdated Show resolved Hide resolved
components/rmrk/Gallery/GalleryItemRelated.vue Outdated Show resolved Hide resolved
components/rmrk/Gallery/GalleryItemRelated.vue Outdated Show resolved Hide resolved
components/rmrk/Gallery/GalleryItemRelated.vue Outdated Show resolved Hide resolved
components/rmrk/Gallery/GalleryItemRelated.vue Outdated Show resolved Hide resolved
components/rmrk/Gallery/GalleryItemRelated.vue Outdated Show resolved Hide resolved
@preschian
Copy link
Member Author

oh, it seems not triggering on route change (homepage to item), checking

@preschian preschian requested a review from vikiival April 1, 2022 16:45
@preschian
Copy link
Member Author

updated, ready for review again 🙏🏻

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.

code looks definitely cleaner

just move the Min to the constants and make sure that all previous comments are resolved :)

components/rmrk/Gallery/GalleryItemCarousel.vue Outdated Show resolved Hide resolved
@preschian
Copy link
Member Author

code looks definitely cleaner

just move the Min to the constants and make sure that all previous comments are resolved :)

yaps, previous comments should be updated also. some of them outdated

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.

DID NOT test 🧪

code is ~oki

@yangwao
Copy link
Member

yangwao commented Apr 2, 2022

Found container 🫙 is broken on mobile again 😬 and it's moving, but I guess we can postpone it to another following issue.

Record_2022-04-02-10-36-02.mp4

@yangwao
Copy link
Member

yangwao commented Apr 2, 2022

pay 300 usd

@yangwao
Copy link
Member

yangwao commented Apr 2, 2022

😍 Perfect, I’ve sent the payout
💵 $300 @ 195.63 USD/KSM ~ 1.534 $KSM
🧗 DY4SQF2iD456tH89aQtz5wv1EV3BbSW8wKKuMcwbmXaj1pM
🔗 0x22f7ec0072454afb40b92070fa0ed6f7c09aecb73511793a1c0e4d9db5151191

🪅 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 2, 2022
@yangwao yangwao merged commit 3dd2c80 into kodadot:main Apr 2, 2022
@preschian
Copy link
Member Author

Found container 🫙 is broken on mobile again 😬 and it's moving, but I guess we can postpone it to another following issue.

Record_2022-04-02-10-36-02.mp4

oops 😱

thanks a lot @yangwao @vikiival

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.

Related strip of NFTs contextual to NFT item
3 participants