feat: mass listing - Githubissues

stephenjason89 commented 9 months ago

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
[x] Feature
[ ] Refactoring

Context

[x] Closes #6941
[ ] Requires deployment <snek/rubick/worker>

Before submitting pull request, please make sure:

[x] My contribution builds clean without any errors or warnings
[x] I've merged recent default branch -- main and I've no conflicts
[x] I've tried to respect high code quality standards
[x] I've didn't break any original functionality

Optional

[ ] I've tested it at </ksm/collection>
[ ] I've tested PR on mobile
[ ] I've written unit tests 🧪
[ ] I've found edge cases

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

[x] Fill up your DOT address: Payout

Community participation

[ ] Are you at KodaDot Ecosystem Telegram?

Screenshot 📸

[ ] My fix has changed something on UI; a screenshot is best to understand changes for others.

Copilot Summary

`🤖 Generated by Copilot at 0c66f2c`

This pull request implements a listing cart feature for NFT owners who want to sell their items. It adds a new stores/listingCart module that defines a Pinia store for managing the cart state and actions, and uses local storage for persistence. It also modifies the ItemsGridImage component to display a button for adding items to the cart.

`🤖 Generated by Copilot at 0c66f2c`

You have the power to sell your art But beware of the listing cart It will sync with your local storage And unleash the Pinia carnage

kodabot commented 9 months ago

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

netlify[bot] commented 9 months ago

Deploy Preview for koda-canary ready!

Name	Link
Latest commit	edd67df1176899c7da7f714907fccb9f1c422618
Latest deploy log	https://app.netlify.com/sites/koda-canary/deploys/64f73f81f7a26c0008189ef4
Deploy Preview	https://deploy-preview-7076--koda-canary.netlify.app
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

reviewpad[bot] commented 9 months ago

AI-Generated Summary: This pull request introduces the feature of listing items in a cart. Changes are made to the file ItemsGridImage.vue, which now includes new functionalities such as listing items for sale and checking if an item is already in the cart. This is possible through using the store listingCart.ts. This new file has been added to manage the listings in the cart. It includes definitions for different actions such as setting an item, updating an item, removing an item, and clearing the cart. These functionalities utilize local storage for persistence. A total of 92 lines have been added in this update spread across two files. The purpose of this update is to enhance the user’s shopping experience by keeping track of their potential purchases.

yangwao commented 9 months ago

Can we try to test it?

Interesting 🤔 progress of sizing From small, through medium till largest

stephenjason89 commented 9 months ago

@yangwao maybe in 3hours i'll be done Just gonna save the transactions 😄

exezbcz commented 9 months ago

hello!

nice job, I will continuously check the pr:

First round

select all should select the cards, giving them the added to list status - blue bg and stroke etc...
this thing should be displayed after at least one item is added:
please check the styling :

clicking floor in the modal does nothing even thought there is the collection floor price :

check dark mode input styling please:

smaller thrash icon here:

otherwise really solid! thank you @stephenjason89, next round soon :D

stephenjason89 commented 9 months ago

hello!

nice job, I will continuously check the pr:

otherwise really solid! thank you @stephenjason89, next round soon :D

Thank you @exezbcz for pointing out the dark theme issue. I am glad you liked the layout and everything was good.

Yup all the other functionalities are not yet implemented but I am almost done with it.

Will push a PR in a while.

stephenjason89 commented 9 months ago

@exezbcz Please check PR. This is almost final. Will just have to save this on the chain now and i will remove this from draft.

Thank you

exezbcz commented 9 months ago

this thing should be displayed after at least one item is added

missing

styling:

left padding for the placeholder please

stephenjason89 commented 9 months ago

this thing should be displayed after at least one item is added

Oh okay, I thought it should always show so a user can do a Select All initially. But yes hiding it is good also, that's what i want to do initially.

missing

styling:

left padding for the placeholder please

noted on this.

stephenjason89 commented 9 months ago

@exezbcz & @prury If you could possibly check and test. Waiting for your feedback.

Thank you so much

stephenjason89 commented 9 months ago

@yangwao Done, very late here now...

😴 🛌 😪

Goodnight guys!

prury commented 9 months ago

RMRK 1 and 2 NFTs not loading here for me:

Clicking floor price just sets everything to 0

stephenjason89 commented 9 months ago

By the way, I am using the floor price returned from the graphql query nftListWithSearch

collection {
      id
      name
      floor
    }

Clicking floor price just sets everything to 0

I believe right now it returns a wrong value. It is returning the lowest value that was set in a collection even if that lowest value was already changed.

I think this is not working as it should @yangwao @roiLeo Please confirm

I would hate to use useCollectionDetails() as this will run another query per collection just to get this information.

RMRK 1 and 2 NFTs not loading here for me:

@prury I might have to re-implement floor as it is caused by our backend. Data returned was not consistent between chains

Other chains didn't have the floor available

I'll just use useCollectionDetails() and re-query the information. This is not optimal though.

stephenjason89 commented 9 months ago

@prury Please check should be perfect now 😄

yangwao commented 9 months ago

kudos for making it, ~~yet it seems floor price +5% doesn't work well? It listed bellow 🤔~~ ah, confused, it works seems good.

yangwao commented 9 months ago

We should auto-fix the decimal point for user?

yangwao commented 9 months ago

Probably for follow-up, we can add another OR strategy to add +5% +10% +15% from users buying price.

in this case if buy was 0.6KSM, 15% add up should be 0.69KSM

exezbcz commented 9 months ago

Next round

close modal when no items

input no active state, please check the component

infinity when no floor and clicking the floor +5% button?

should stay empty

potential earnings not calculating

stephenjason89 commented 9 months ago

kudos for making it

Thanks boss ❤️

Probably for follow-up, we can add another OR strategy to add +5% +10% +15% from users buying price.

Will do.

We should auto-fix the decimal point for user?

Noted on this, how many decimal places do we need? and do we need to round or just use the toFixed? Also, is this only for the display, what i meant is, for the computation on use floor price or +-5% Should I use the original floor price?

How can i reproduce this?

@exezbcz noted on this. https://github.com/kodadot/nft-gallery/pull/7076#issuecomment-1703819090 Thanks

@roiLeo Thanks for pointing out the css helpers, is there a documentation where i can read more on this? Also it would also be nice if I can see a documentation of our styles & components so I can reuse as much as possible.

hello, some changes I would like to see:

more translated string

Will use the i18n and put up english translation. what languages do you want to include? Is google translate acceptable?

Arrow functions check out other stores to use same function pattern

As much as possible I use arrow functions where I can, though this

Is not possible since the this context is different, in this case, inside the pinia getters.

Let me explain a bit more

won't work because itemsInChain is a getter

will not work as well because it inherits the this context

In this method, the this context is dynamically bound, meaning that when you call count, this refers to the object that owns the count method. This will correctly reference itemsInChain getter.

please don't use set when it's not needed

I don't use set when it is not needed. I used it inside the listingCart store because I am creating a property and vue doesn't track the properties created on an object on the fly

if we will not use Vue.set another option here is to define the initial property which may cause more lines of code and is not needed in Vue 3

or to replace the whole object which in this case is the state of the pinia store.

I can switch it if we really do not wish to use Vue.set. On vue 3 however, replacing this with Object.assign will be suffice.

But yeah, I'll just refactor it to not use set .

Thank you

@Jarsen136 Thanks I will try to use more of the es6 forEach

Thank you everyone for commenting and suggesting things for improvement. I appreciate all of your time and effort to check my PR ❤️

yangwao commented 9 months ago

How can i reproduce this?

One input used ,, one ., but probably we should handle it globally iirc

stephenjason89 commented 9 months ago

Hello everyone I have finished everything here. @exezbcz only thing i can't reproduce is the infinity, but i changed it a bit so please let me know if it still happens.

@yangwao I have reimplemented how +-5% works. Please let me know if you like it this way instead. You can recursively add more 5% or less 5%

By the way @yangwao we need to correct this on the backend, if it's possible that we can return a correct floor value for all chains then we won't need to requery for every collection using useCollectionDetails() I believe this is redundant.

collection {
      id
      name
      floor
    }

Also, i used the default settings of formatBalance which is 4 decimal places.

vikiival commented 9 months ago

we need to correct this on the backend, if it's possible that we can return a correct floor value for all chains then we won't need to requery for every collection using useCollectionDetails() I believe this is redundant.

Can you rephrase your question please? @stephenjason89

stephenjason89 commented 9 months ago

we need to correct this on the backend, if it's possible that we can return a correct floor value for all chains then we won't need to requery for every collection using useCollectionDetails() I believe this is redundant.

Can you rephrase your question please? @stephenjason89

for example in the graphql query

It would be nice to return floor here for every chain. So that in the front-end we can just use this directly instead of using the composable

const { stats } = useCollectionDetails({
  collectionId: props.nft?.collection?.id || props.nft?.collectionId,
})

then using stats.value.collectionFloorPrice

Right now, only the

can return floor but it is the wrong floor value so we also need to check this.

Example returned data with floor

for RMRK1 / 2 and the others it doesn't have a floor field so it will throw a graphql error on this

This causes to have 1 more query per collection. Which we could have avoided if it was already returned by the first query.

vikiival commented 9 months ago

for RMRK1 / 2 and the others it doesn't have a floor field so it will throw a graphql error on this

Well the only indexer that does not have floor is ksm that is blocked by - https://github.com/kodadot/nft-gallery/issues/5853

https://squid.subsquid.io/marck/v/v3/graphql

rmrk has floor just we need to update indexer to newer version 👍

https://squid.subsquid.io/rubick/v/v9/graphql

stephenjason89 commented 9 months ago

for RMRK1 / 2 and the others it doesn't have a floor field so it will throw a graphql error on this

Well the only indexer that does not have floor is ksm that is blocked by - https://github.com/kodadot/nft-gallery/issues/5853

Oh was rmrk recently added?

https://github.com/kodadot/nft-gallery/pull/7076#issuecomment-1703277279 It was throwing an error a few days ago Didn't try today.

https://squid.subsquid.io/marck/v/v3/graphql
rmrk has floor just we need to update indexer to newer version 👍
https://squid.subsquid.io/rubick/v/v9/graphql

Got it.

But you have to check it, It is incorrectly returning the floor value. It returns the lowest value that was listed on the collection but if i change the lowest value to something higher then the query still returns the lowest value that was ever set before.

Example Collection 1 has Nft 1 - price 1.89 Nft 2 - price 3.5 Nft 3 - price 4.6

The floor is 1.89

But if i change nft 1 price to 5.5

It will still return floor 1.89 When it should be 3.5 now

If you lower the price of nft 1 to 0.5

Then floor correctly gives me 0.5

vikiival commented 9 months ago

Is this bug happenning on all indexers or just rmrk or ksm

If so please open the issue For all: kodadot/loligo For ksm/rmrk: kodadot/rubick

I try to find time to fix it or can guide.

Edit: most probably i know where is the the issue.

stephenjason89 commented 9 months ago