-
Notifications
You must be signed in to change notification settings - Fork 10
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
Implement shopping cart #128
Conversation
Amazing! @amenconi - I'll be reviewing the code more in details over the weekend. @SoonaverseTF - This new feature is available here: https://amenconi-shopping-cart.app-cqo.pages.dev - please work with the community on the testing and once you approve this request it'll be merged. It's linked to PROD database. See more on reason why here: #127 (review) @SoonaverseTF - please also determine the TOKEN reward so we can update CONTRIBUTORS.md and execute SOON distribution |
Thanks Adam. Initially dev link for testing seemed to work well yesterday when I tried, now it seems that no data loads (i.e. all collections show 0 deposited for all I've checked). When I checked it yesterday I found 2 issues that might be worth correcting before merging anything:
Let me know what would be best practice to address each of these issues and will keep testing to see if I find anything else. Thank you. |
Pushed more refinements to branch, namely:
This should allow a simplistic method of sweeping for users. In the future we should probably discuss adding in a more robust sweep mechanic with such features as:
Thank you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great effort! See some of my comments.
I'll talk to people in #dev channel and ask them to help on testing. I'll try to do some testing myself as well tomorrow based on all features you highlighted.
src/app/@core/services/filter-storage/filter-storage.service.ts
Outdated
Show resolved
Hide resolved
src/app/components/cart/components/cart-modal/cart-modal.component.html
Outdated
Show resolved
Hide resolved
src/app/components/nft/components/nft-card/nft-card.component.ts
Outdated
Show resolved
Hide resolved
Yes this is a good point. I was just thinking about NFTs posted with specified "saleAccessMembers" as well. I'll have to more closely review all possible states that might not allow the sale of the NFT for both logged in and not logged in users and add it in for a more comprehensive "isAvailableForSale" boolean check. |
I've been testing and overall it's looking great and working fine.
|
Deploying prod-soonaverse-pr-previews with Cloudflare Pages
|
Sorry for delay on things. Thank you for the fantastic feedback. I've just pushed a large revision and will try to capture the major changes. I refactored the cart checkout modal to gain better control of when and how it opens and closes. Main feature changes:
There is likely more that we can do to refine and tweak but this should get us much closer to where we need it. Let me know what you guys think. Thanks! |
Great update!
|
@amenconi - FYI I've approved the current code but I'll review any further changes based on above feedback. Great progress! |
Ah gotcha, I've added that and also included total price with USD conversion in the network group header. This latest commit implements everything in last message:
I've also included the following:
Unfortunately it looks like the test link for Cloudflare Pages was not successful but once that can be fixed please test and let me know if any further issues are found. Thank you! |
Awesome Alec, thank you for this update! @adamunchained can you please take a look at the cloudflare build failure? Thanks! |
@amenconi, I made new tests and so far so good. Only one minor thing that can be improved (see image attached) For the first item, I put the value "123" but only "12" is read. Maybe enlarge the field a bit so that more characters can fit in. |
Excellent, glad this latest version seems to be working well. Nice catch for collections with large available amounts, will make sure it's large enough for at least 5 figures. Also as a continuation of PR 127 discussion, it's odd it's not rounding down, it seemed to round up and down without issue in debug but will take another look and try to make sure valid figures are displayed at all times. Also will make sure NFT image doesn't change shape as your picture showed and will add the USD value for single and multi NFT buy previews. @adamunchained thank you for merging 127 and 128! I'll get the merge conflicts solved, finish the above items and add in the ability on NFT page when choosing quantities of more than 1 to also be added to cart as well as checking out directly. |
…ended cart qty input width, add multiple NFTs to cart from NFT page
I believe PR 127 should now be merged with 128. I've also made the following modifications:
I'll look into the Bloom payment icon/link more but I believe that was likely added to a branch that wasn't merged with this one yet. If needed though I will add it. Thank you! |
Hi @amenconi , I found a visual bug in the cart checkout process, here is how to reproduce it:
Then, I switched tabs again and went back to the first tab (where I had confirmed the checkout process from initially) and suddenly the Bloom button disappeared there: |
Hi @emmap3-do, So if I understand correctly this appears to be 2 issues:
This is likely due to cart changes and state being stored and shared between tabs via local storage, but checkout meta data (selected network for purchase in this case) only being stored in local memory. I should be able to solve this by setting and retrieving this data from local storage as well. To keep things clean I'll see if I can extend cart local storage to include this type of meta data as well so we aren't using a ton of separate local storage items to keep track of.
Unclear to me if these items are related or not but will review all code part of this PR and verify the bloom link is present everywhere you'd expect it to be. Will update once fixed, thank you! |
Exactly, thanks Alec! |
…ow real time updates for these across browser tabs
With this latest push I actually went the opposite direction than I thought and created local storage items for "selected network" and "current step" as I realized we should change and track these separately to cart changes. Persisting states for these in local storage is necessary for real time updates across browser tabs. I've also reverted intentional behavior with the cart checkout modal. Originally if you opened the checkout modal and made a network selection but didn't lock in the transaction, then closed the modal and re-opened it, it would wipe out the network selection and start the selection process over. I had to ditch this as it caused issues with multiple browser tabs open, since if both had the cart checkout modal open and one was closed it would wipe out the selected network on the second open tab. So now it will remember the network selection until the transaction is finished or expired. Also looked at the Bloom deep link. My code calls "wen-wallet-deeplink" component which has logic that generates a link for Firefly and TanglePay as long as there is an input of valid target address and target amount, however the logic to generate a Bloom link requires an input of valid network. Since the second tab wasn't getting real-time updates on selected network, this input was blank and so was only generating the other 2 deeplinks. Persisting selected network with local storage (and as an observable) should solve this. Thank you! |
Found another bug. I added SMR and IOTA networks NFTs to my cart. Then I started the checkout process for IOTA network and waited for it to timeout. After that, the IOTA item got stuck as shown below, instead of being enabled for removal from cart: Other than that, very small couple of things: |
…purchase transaction plus some minor UI styling tweaks.
Newest commit changes:
Thanks! |
Sweep to Cart - I'm not sure how the next scenario is expected to work, neither if this needs to be addressed now or can wait for a future release. If you go to a collection that has not mint out, eg: For such cases, I'm not sure if it would make more sense to just mint 3 new NFTs, or to skip new mints and sweep 3 from floor minted and put on sale by someone else... Cart checkout - The total count of items in the summarized title is not accurate when you have several NFTs to be minted added to cart, as shown below: A third thing to check is: add NFTs to cart, open cart details popup and leave it open. Open up a new browser tab and in there, open up the cart details popup too. On this 2nd tab, click 'Checkout' button, then 'Confirm and lock'. Now go to the 1st tab and you will be able to make edits to the cart items (eg you can increase/decrease qty, or you can remove items from cart. You can make these edits and then if you swap again to the other tab, it is reflected in the title summary too (NFTs total qty and price are updated). This could be missleading for the user. |
Good question on the sweep to cart, we can definitely change the logic to handle either scenario (multiple quantity of mint NFT or ignore minting NFTs altogether). I'll pose the question on Discord and see if we can get others to weigh in on which direction is preferable. I'll change the cart checkout title to have the full count of NFTs rather then the "unique" count being checked out. For the third issue, sounds like the broadcast of changes to current step, pending checkout transaction and other observables involved in locking down cart items that are part of a checkout process are either not going out as expected or not being read reactively in some cases. Will experiment a bit and make sure the qty changes, item removals and cart clearing is updating based on checkout processes happening on another tab in real-time. Thank you! |
Changes in latest push:
The cross tab state management refactor was more involved than I originally thought it would be. I did some general testing and it appears to work and I don't think it broke any other pieces but please let me know how it works for you now. Thank you. |
Great effort @emmap3-do / @amenconi !! I've presented your feature to SOON Committee (request https://github.com/soonaverse/foundation/issues/83) and it has just been approved by a majority. It has been agreed to give away following allocation:
Please provide your SMR address so I can update CONTRIBUTORS.md in this pull request and merge into master. We can then release this feature into the production. I'm sure community will be excited to use it. Great effort team and thank you for your amazing work! It's very much appreciated. Looking forward to see more work from you! |
Thank you @adamunchained and SOON committee! Additionally can't stress enough the huge appreciation to emmap3 for their assistance throughout this development. Very much looking forward to supercharging our efforts to bring more improvements as fast as we can, especially now, as we become more familiar with the code base. SMR address: smr1qrncyy5lcfpr4hta0hg7qp2cmw6ssm0ycllx5nnz5pwcup8rxs0zzp2jp64 |
Thank you Adam and SOON Commitee, and Thank you Alec for bringing new features into the platform! My SMR address remains the same: |
Apologies, didn't realize changing branch name would close the previous PR.
Initial shopping cart main feature set:
Left to do:
Review responsive design for small viewports and modify UI accordingly.