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

fix: Current generative drops are not unlockable #7862

Merged
merged 8 commits into from
Nov 1, 2023

Conversation

Jarsen136
Copy link
Contributor

@Jarsen136 Jarsen136 commented Oct 28, 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

  • Bugfix
  • Refactoring

Needs QA check

  • @kodadot/qa-guild please review

Context

Did your issue had any of the "$" label on it?

Screenshot 📸

  • My fix has changed UI

Copilot Summary

🤖 Generated by Copilot at af98ea8

This pull request refactors the drop components to simplify their structure and extract common elements, such as the unlockable item info, into reusable components. It also introduces a new component CollectionUnlockableItemInfo that displays the unlockable content explanation based on the collection id. Additionally, it modifies the useUnlockable composable to accept a more generic type for the entity parameter. These changes improve the code readability, maintainability, and flexibility.

🤖 Generated by Copilot at af98ea8

We are the masters of the code, we break the duplication
We extract the logic, we create the CollectionUnlockableItemInfo
We use the script setup, we unleash the power of Vue
We refactor the drops, we make them shine like new

@netlify
Copy link

netlify bot commented Oct 28, 2023

Deploy Preview for koda-canary ready!

Name Link
🔨 Latest commit bc14df8
🔍 Latest deploy log https://app.netlify.com/sites/koda-canary/deploys/6541079e5220e100080d7729
😎 Deploy Preview https://deploy-preview-7862--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 configuration.

@kodabot
Copy link
Collaborator

kodabot commented Oct 28, 2023

WARNING @Jarsen136 PR for issue #7843 which isn't assigned to you. Please be warned that this PR may get rejected if there's another assignee for issue #7843

@reviewpad
Copy link
Contributor

reviewpad bot commented Oct 28, 2023

AI-Generated Summary: This pull request addresses the issue with the current generative drops not being unlockable. The changes involve modifications in multiple files. A significant portion of code was removed, refactored, and consolidated into a new Vue component called UnlockableItemInfo.vue. This new component contains the necessary details and functionality about how the unlockable items work.

The component is linked to the DropContainer.vue, Generative.vue, UnlockableContainer.vue, and voteDrop/DropContainer.vue files replacing redundant code.

The useUnlockable.ts file was also updated to account for additional parameters. Overall, 150 lines of code were deleted and 57 new lines were inserted across 6 different files.

@reviewpad reviewpad bot added the medium Pull request is medium label Oct 28, 2023
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like the api is checking if the nft is the unlockable item. Is there any idea to check if the collection is unlockable? @vikiival
#7843 (comment)
resolve/${prefix}-${nftId}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vikiival Could you please take a look at it?

Copy link
Member

Choose a reason for hiding this comment

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

@vikiival Could you please take a look at it?

You have been pranked by bad naming

Usage of keywise is

const { isUnlockable, unlockLink } = useUnlockable(collection)

Therefore it is checking as

GET /resolve/${prefix}-${collectionId}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

LOL Thank you for pointing it out. I would correct the bad naming to avoid confusion.

@Jarsen136 Jarsen136 marked this pull request as ready for review October 30, 2023 16:36
@Jarsen136 Jarsen136 requested a review from a team as a code owner October 30, 2023 16:36
@Jarsen136 Jarsen136 requested review from roiLeo and floyd-li and removed request for a team October 30, 2023 16:36
@prury
Copy link
Member

prury commented Oct 30, 2023

what about this section? should we hide it for non unlockables as well?
image

it seems now that no drop is showing this part:

image

as far as i know waifu drops should have and generative and kodachain should not

@prury prury added the S-changes-requested-🤞 PR is almost good to go, just some fine tunning label Oct 30, 2023
@Jarsen136
Copy link
Contributor Author

what about this section? should we hide it for non unlockables as well? image

it seems now that no drop is showing this part:

image

as far as i know waifu drops should have and generative and kodachain should not

You're right. I have fixed them.

@prury
Copy link
Member

prury commented Oct 31, 2023

last thing, since you've removed the unlockable section, kodachain drop needs some spacing between the footer
image

Copy link

codeclimate bot commented Oct 31, 2023

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

View more on Code Climate.

Copy link

sonarcloud bot commented Oct 31, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
1.0% 1.0% Duplication

@Jarsen136
Copy link
Contributor Author

last thing, since you've removed the unlockable section, kodachain drop needs some spacing between the footer image

✅ Added some spacing between them.

@prury prury added S-works-for-me-✅ qa-guild has tested PR from end user perspective and functionality worked and removed S-changes-requested-🤞 PR is almost good to go, just some fine tunning labels Oct 31, 2023
@vikiival
Copy link
Member

kodachain drop needs some spacing between the footer

KodaChain should be unlockable 🤔

@prury
Copy link
Member

prury commented Oct 31, 2023

kodachain drop needs some spacing between the footer

KodaChain should be unlockable 🤔

what it should unlock? because for the waifu mints we unlock $ vouchers, but the kodachain we just give ppl the nft in case they voted for proposal

@yangwao
Copy link
Member

yangwao commented Nov 1, 2023

Thanks!
pay 30 usd

@yangwao yangwao merged commit 6fe7371 into kodadot:main Nov 1, 2023
14 checks passed
@yangwao
Copy link
Member

yangwao commented Nov 1, 2023

😍 Perfect, I’ve sent the payout
💵 $30 @ 4.34 USD/DOT ~ 6.912 $DOT
🧗 16SjUbGKSdjCdWTy3NNT3JxbRVGGqD4mwkHpc6BD9U2Rp29Z
🔗 0x73a75117f4db8167c2fd67ac4725a369f97f671046d2d9f9b40520ddf0ba5ed4

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

@yangwao yangwao added paid pull-request has been paid and removed paid pull-request has been paid labels Nov 1, 2023
@yangwao
Copy link
Member

yangwao commented Nov 1, 2023

pay 30 usd

@yangwao
Copy link
Member

yangwao commented Nov 1, 2023

😍 Perfect, I’ve sent the payout
💵 $30 @ 4.35 USD/DOT ~ 6.897 $DOT
🧗 16SjUbGKSdjCdWTy3NNT3JxbRVGGqD4mwkHpc6BD9U2Rp29Z
🔗 0x8393c6a8c499b02ca0001fc6c275c84801c776075031b5a48f1267514067c038

🪅 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 Nov 1, 2023
This was referenced Nov 1, 2023
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-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.

Current generative drops are not unlockable
5 participants