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

feat: Follow up for success drop notification #6256

Merged
merged 14 commits into from
Jun 23, 2023

Conversation

Jarsen136
Copy link
Contributor

@Jarsen136 Jarsen136 commented Jun 16, 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
  • Feature
  • Refactoring

Needs Design check

Context

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

Screenshot 📸

  • My fix has changed UI

tested on /stmn/unlockable?redesign=true

image image

twitter
image

Copilot Summary

🤖 Generated by Copilot at f4f1c7b

Added feature flag and mock code for unlockable NFTs and improved UX for minting and sharing them. The changes affect the UnlockableContainer and UnlockableLoader components in components/collection/unlockable.

🤖 Generated by Copilot at f4f1c7b

Sing, O Muse, of the skillful code that changed the face of NFTs
How the clever developers added a feature flag and mock code
To test the new design of the UnlockableContainer component
And make it easier to mint and share the hidden treasures of art

@Jarsen136 Jarsen136 requested a review from a team as a code owner June 16, 2023 17:06
@Jarsen136 Jarsen136 requested review from preschian and roiLeo and removed request for a team June 16, 2023 17:06
@kodabot
Copy link
Collaborator

kodabot commented Jun 16, 2023

SUCCESS @Jarsen136 PR for issue #6252 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 Jun 16, 2023

Deploy Preview for koda-canary ready!

Name Link
🔨 Latest commit 57fcaee
🔍 Latest deploy log https://app.netlify.com/sites/koda-canary/deploys/64902eaaa977cf0008308f93
😎 Deploy Preview https://deploy-preview-6256--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 Jun 16, 2023

AI-Generated Summary: This pull request introduces a feature for displaying a success notification upon successfully dropping an NFT. It modifies two components, UnlockableContainer.vue and UnlockableLoader.vue, to handle the new functionality. The changes include adding a countdown timer, updating the UI text and layout, and providing a Twitter sharing link for users to share their success.

@reviewpad reviewpad bot added small Pull request is small waiting-for-review labels Jun 16, 2023
@Jarsen136
Copy link
Contributor Author

Jarsen136 commented Jun 16, 2023

Interesting, the build always failed.
image

I guess it's related to this Merge #6201
There is the same error https://github.com/kodadot/nft-gallery/actions/runs/5281682251/jobs/9555596775

image

@Jarsen136
Copy link
Contributor Author

Jarsen136 commented Jun 17, 2023

I guess the failed error is related to some wired build cache because it builds successfully on my self-deploy env.

Preview env:
https://issue-6252--thriving-bunny-03a5e8.netlify.app/stmn/unlockable?redesign=true

@Jarsen136 Jarsen136 requested a review from exezbcz June 17, 2023 18:00
@preschian
Copy link
Member

I guess the failed error is related to some wired build cache because it builds successfully on my self-deploy env.

on cypress/e2e should be fixed with this #6265
on netlify, I think error on their side at that time. seems like fixed for now

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.

  • i18n
  • redesign

✅ otherwise lgtm

components/collection/unlockable/UnlockableLoader.vue Outdated Show resolved Hide resolved
components/collection/unlockable/UnlockableLoader.vue Outdated Show resolved Hide resolved
@exezbcz
Copy link
Member

exezbcz commented Jun 18, 2023

Hey, hey, I have a few changes.

First, I would change the copy; the first one I gave you must be corrected.
Before: "Get ready for the big reveal! Your NFT will be visible in 30 seconds."
After: "Get ready for the big reveal! Your NFT is visible now."

bold: 30 seconds, visible now

  • let's make the counter dynamic so that it will change every second. 29,28,....

  • It will be good if you can share your success on Twitter in the waiting time - that means the share link will be visible even if you are waiting for the nft to be ready. @vikiival said it is possible to display the image you have selected in the tweet. The link in the tweet will redirect to the unlockable page. Please let me know if I am wrong.

Thanks!

@Jarsen136
Copy link
Contributor Author

  • It will be good if you can share your success on Twitter in the waiting time - that means the share link will be visible even if you are waiting for the nft to be ready. @vikiival said it is possible to display the image you have selected in the tweet. The link in the tweet will redirect to the unlockable page. Please let me know if I am wrong.

Thanks!

I have explained the reason why we could not attach the image auto on this comment #6252 (comment)
You could also check this answer https://stackoverflow.com/questions/18321649/twitter-web-intent-to-post-image. We could not display the image auto by the redirect link, except the user attaches it manually.

Except for the changes in the image display, the rest has been completed ✅
image
image

@exezbcz
Copy link
Member

exezbcz commented Jun 18, 2023

@Jarsen136 ah oki, nevermind, my bad :D

Could you please make the countdown dynamic so that every second it lowers and lowers? It will give the user a more interactive feeling.

  • I think that the cross has disappeared
    image
  • or is it just me? I am opening it through the link
  • issue-6252--thriving-bunny-03a5e8.netlify.app/stmn/unlockable?redesign=true

I believe that is the last thing.

@Jarsen136
Copy link
Contributor Author

Could you please make the countdown dynamic so that every second it lowers and lowers? It will give the user a more interactive feeling.

The minting time is estimated, which means it's not equal to 30 seconds or a specific number.

  • I think that the cross has disappeared

It's related to the icon display on the self-deploy preview env. After merging this PR, I bet the cross icon will be back as usual.
I have tested it on my local env.
image

@exezbcz
Copy link
Member

exezbcz commented Jun 18, 2023

The minting time is estimated, which means it's not equal to 30 seconds or a specific number.

Yes :D I wanted to make it an actual countdown, not a still number. It's better for the user. I know it stays the same for the minting time etc.

It's related to the icon display on the self-deploy preview env. After merging this PR, I bet the cross icon will be back as usual.
I have tested it on my local env.

oki nice!

@Jarsen136
Copy link
Contributor Author

Jarsen136 commented Jun 18, 2023

The minting time is estimated, which means it's not equal to 30 seconds or a specific number.

Yes :D I wanted to make it an actual countdown, not a still number. It's better for the user. I know it stays the same for the minting time etc.

Sorry, but I do not think it's a good idea to make it a countdown. For example, if we set the estimated time to 30s, the minting may be finished in a time longer than 30s, in this case, the number would change from 30 to 0 seconds and stay at zero, then the user may feel more confused. WDYT?

@exezbcz
Copy link
Member

exezbcz commented Jun 18, 2023

@Jarsen136 yes, agree, but we set the 30s, so there is a really high probability that it's going to be finalized.

@Jarsen136
Copy link
Contributor Author

@Jarsen136 yes, agree, but we set the 30s, so there is a really high probability that it's going to be finalized.

Alright, let me make a countdown for it.

@exezbcz
Copy link
Member

exezbcz commented Jun 18, 2023

@Jarsen136 Yes, if it wont work well in the future :D my fault

Thanks, sir!

@Jarsen136
Copy link
Contributor Author

@Jarsen136 Yes, if it wont work well in the future :D my fault

Thanks, sir!

You're welcome. The countdown should work now. Please check it.

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.

translations otherwise oki

components/collection/unlockable/UnlockableContainer.vue Outdated Show resolved Hide resolved
@Jarsen136 Jarsen136 requested a review from vikiival June 19, 2023 02:01
@codeclimate
Copy link

codeclimate bot commented Jun 19, 2023

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

View more on Code Climate.

@prury
Copy link
Member

prury commented Jun 22, 2023

I'm not testing this one since the redesign variable already got removed and @exezbcz has thoroughly tested

@vikiival vikiival merged commit c6e2b82 into kodadot:main Jun 23, 2023
This was referenced Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Follow up for success drop notif
7 participants