kodadot / nft-gallery

Generative Art Marketplace
https://kodadot.xyz
MIT License
605 stars 347 forks source link

#4437 cancel accept offer #5052

Closed prachi00 closed 1 year ago

prachi00 commented 1 year ago

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 πŸ“Έ

kodabot commented 1 year ago

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

netlify[bot] commented 1 year ago

Deploy Preview for koda-nuxt ready!

Name Link
Latest commit 0a7972750f5e2aaab130c00743ec24a94b94c3ef
Latest deploy log https://app.netlify.com/sites/koda-nuxt/deploys/63fce14c7afdbb00085a393a
Deploy Preview https://deploy-preview-5052--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.

prachi00 commented 1 year ago

Known issue: even though I did refetch and the fetchPolicy: 'network-only', , it still doesnt give the updated status after canceling/accepting offer and it still gives active On reload, it is fine.

Jarsen136 commented 1 year ago

Known issue: even though I did refetch and the fetchPolicy: 'network-only', , it still doesnt give the updated status after canceling/accepting offer and it still gives active On reload, it is fine.

There are two approaches to solving it:

  1. refetch graphql query after submiting offer

    const { data, refetch } = useGraphql({  xxx })
  2. usinguseSubscriptionGraphql composables api

    composables/useSubscriptionGraphql.ts
prachi00 commented 1 year ago

3. usinguseSubscriptionGraphql composables api

@Jarsen136 I did the 1st approach, but for some reason it refetches but in response it still gives 'active' status when it should be 'cancelled'

Jarsen136 commented 1 year ago
  1. usinguseSubscriptionGraphql composables api

@Jarsen136 I did the 1st approach, but for some reason it refetches but in response it still gives 'active' status when it should be 'cancelled'

I guess the status has not been updated on the indexer side before you do refetch. You may try to fetch it later or do the 2rd approach.

exezbcz commented 1 year ago

Can we also please remove status column? I think it does not have to be there, because seeing expired offer has no value for you. There would also be more space for the buttons.

exezbcz commented 1 year ago

yes, forgot to mention dark mode buttons

image

I put them into gallery item handoff figma

They change text color and bg on hover to k-accentlightdark

Thank you!

exezbcz commented 1 year ago
image
roiLeo commented 1 year ago
  • you should not see cancel offer option if you are not the one that placed it?

If you are the current owner of Nft, you should be able to withdraw incoming offers ref #4239

small note: why don't we use icon (check & cross) instead of text?

exezbcz commented 1 year ago
  • you should not see cancel offer option if you are not the one that placed it?

If you are the current owner of Nft, you should be able to withdraw incoming offers ref #4239

small note: why don't we use icon (check & cross) instead of text?

Hmm, I dont get it, why you have to have option to cancel incoming offer from others? You can only cancel offer you are the one that made it.

ad icons - because I did not count on having 2 buttons there.

vikiival commented 1 year ago

You can only cancel offer you are the one that made it.

Sadly, no

According to the implementation: current nft owner or offer maker can cancel the offer

roiLeo commented 1 year ago

Hmm, I dont get it, why you have to have option to cancel incoming offer from others? You can only cancel offer you are the one that made it.

side note: We are limited by 1 offer by Account per NFT.

prachi00 commented 1 year ago

To do in another issue:

exezbcz commented 1 year ago

@roiLeo @vikiival ah, okay, thanks for clarification I guess its ok to have two buttons there, I will create another issue once I make design that provides variation with both options for the owner. Now it's definitely better than nothing.

one thing is maybe remove the status column - more space

prachi00 commented 1 year ago

@roiLeo

I figured since the ui is different and it can be reused in gallery as well for breadcrumbs, might as well make a new component, what do you think?

roiLeo commented 1 year ago

I figured since the ui is different and it can be reused in gallery as well for breadcrumbs, might as well make a new component, what do you think?

Don't know how you'll use it as breadcrumb filter since it's not suposed to be buttons. IMO, they will look something like this

prachi00 commented 1 year ago

the ui is exactly same though right, it just has a cross, so we can maybe rename the component to something else that can be used in both the places? maybe NeoTags?

roiLeo commented 1 year ago

the ui is exactly same though right, it just has a cross, so we can maybe rename the component to something else that can be used in both the places? maybe NeoTags?

Yup NeoTag seems good to me but I'm not sure it's going to work well with a cross, here's how I see this component: Button content must be clickable on all its area while on NeoTag we should only be able to click on the cross.

ref #4533

prachi00 commented 1 year ago

the ui is exactly same though right, it just has a cross, so we can maybe rename the component to something else that can be used in both the places? maybe NeoTags?

Yup NeoTag seems good to me but I'm not sure it's going to work well with a cross, here's how I see this component: Button content must be clickable on all its area while on NeoTag we should only be able to click on the cross.

ref #4533

ooh yeah you are right, I'll just change it to buttons then for now

prachi00 commented 1 year ago

the ui is exactly same though right, it just has a cross, so we can maybe rename the component to something else that can be used in both the places? maybe NeoTags?

Yup NeoTag seems good to me but I'm not sure it's going to work well with a cross, here's how I see this component: Button content must be clickable on all its area while on NeoTag we should only be able to click on the cross. ref #4533

ooh yeah you are right, I'll just change it to buttons then for now

the ui is exactly same though right, it just has a cross, so we can maybe rename the component to something else that can be used in both the places? maybe NeoTags?

Yup NeoTag seems good to me but I'm not sure it's going to work well with a cross, here's how I see this component: Button content must be clickable on all its area while on NeoTag we should only be able to click on the cross. ref #4533

ooh yeah you are right, I'll just change it to buttons then for now, I'll update this tomorrow for sure

vikiival commented 1 year ago

pay 50 usd

vikiival commented 1 year ago

pay 50 usd

yangwao commented 1 year ago

😍 Perfect, I’ve sent the payout πŸ’΅ $50 @ 38.98 USD/KSM ~ 1.283 $KSM πŸ§— EzGc4s9PgCPx1YnF3fqzhLzVHpHMTL4LWPScwpDrR8JKgSU πŸ”— 0xae1d6c479fa34b95979be2300df3eba91a6c12eb3f3b3dfd9bca6c1158bc4932

πŸͺ… Let’s grab another issue and get rewarded! πŸͺ„ github.com/kodadot/nft-gallery/issues

yangwao commented 1 year ago

😍 Perfect, I’ve sent the payout πŸ’΅ $50 @ 38.98 USD/KSM ~ 1.283 $KSM πŸ§— EzGc4s9PgCPx1YnF3fqzhLzVHpHMTL4LWPScwpDrR8JKgSU πŸ”— 0x17802d9d9939f3003eda34e852053511e00ab854b2b7d69324d0ce2701861677

πŸͺ… Let’s grab another issue and get rewarded! πŸͺ„ github.com/kodadot/nft-gallery/issues

vikiival commented 1 year ago

@prachi00 can you please send 50$ back? Looks like my internet glitched

prachi00 commented 1 year ago

@vikiival seems both of the transfers failed

vikiival commented 1 year ago

@vikiival seems both of the transfers failed

Yeah found that with @yangwao yesterday

codeclimate[bot] commented 1 year ago

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

Here's the issue category breakdown:

Category Count
Complexity 1
Duplication 4

View more on Code Climate.

prachi00 commented 1 year ago

@vikiival can we retry payment on this?

vikiival commented 1 year ago

There is refferenced issue where yangwao would process it

yangwao commented 10 months ago

pay 50 usd

yangwao commented 10 months ago

😍 Perfect, I’ve sent the payout πŸ’΅ $50 @ 5 USD/DOT ~ 10 $DOT πŸ§— 13Qx65nLd6SwdtjrRyuoEtp9CKXhF651xdHBPaXcvhwKm4N1 πŸ”— 0x83878bf19bae557e7b70d02341e59964fb527007645e5122de5c3f667b7e520b

πŸͺ… Let’s grab another issue and get rewarded! πŸͺ„ github.com/kodadot/nft-gallery/issues