kodadot / nft-gallery

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

feat: Control return to confirm action #10306

Closed Jarsen136 closed 3 weeks ago

Jarsen136 commented 1 month ago

Thank you for your contribution to the Koda - Generative Art Marketplace.

👇 __ Let's make a quick check before the contribution.

PR Type

Needs Design check

Needs QA check

Context

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

Screenshot 📸

netlify[bot] commented 1 month ago

Deploy Preview for koda-canary ready!

Name Link
Latest commit 806c6245beb8ddd59f74a6d9f51ea56e33f769c3
Latest deploy log https://app.netlify.com/sites/koda-canary/deploys/664e087472e29a00078cc88b
Deploy Preview https://deploy-preview-10306--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.

prury commented 4 weeks ago

bug or a feature :)

probably we should only allow that shortcut if the transaction has not started CleanShot.2024-05-16.at.17.18.47.mp4

can useMagicKeys combinations give you a cleaner solution?

thank you very much for checking it @hassnian

Jarsen136 commented 4 weeks ago

bug or a feature :)

probably we should only allow that shortcut if the transaction has not started

can useMagicKeys combinations give you a cleaner solution?

Thank you! I have updated it.

prury commented 4 weeks ago

What should be the correct behavior? working only for confirmation buttons on modals or also for trying again when a transaction is cancelled/fails?

Transfer: doesn't work on confirmation button prob because it uses a different modal Teleport bridge: works on try again Drop: works on modal and on try again Create NFT/Collection: won't proceed to signing if you hit it once, i think it's a focus issue, doesn't work on try again Buy Now: works for signing but doesn't for try again Listing: works for signing but doesn't for try again

Jarsen136 commented 3 weeks ago

What should be the correct behavior? working only for confirmation buttons on modals or also for trying again when a transaction is cancelled/fails?

I prefer it only works for confirmation buttons on modal.

Transfer: doesn't work on confirmation button prob because it uses a different modal

Yup, it's a separate modal. However, I have also made the transfer confirm modal work with the shortcut. Now it should work as the others.

Teleport bridge: works on try again Drop: works on modal and on try again Create NFT/Collection: won't proceed to signing if you hit it once, i think it's a focus issue, doesn't work on try again Buy Now: works for signing but doesn't for try again Listing: works for signing but doesn't for try again

Only applying to the confirm button should be enough. I can't manage to go to the page with "try again" button after I cancel the transaction request. It keeps loading all the time. I would leave it to another follow-up issue if anyone else could reproduce it and fix it.

Kapture 2024-05-20 at 01 03 48

hassnian commented 3 weeks ago

modal animation is gone

this pr

https://github.com/kodadot/nft-gallery/assets/44554284/ae5ba16f-7caf-46b3-92eb-530a476c1787

canary

https://github.com/kodadot/nft-gallery/assets/44554284/19daf17d-7c40-4b31-9da0-648cef8340de

codeclimate[bot] commented 3 weeks ago

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

View more on Code Climate.

sonarcloud[bot] commented 3 weeks ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

prury commented 3 weeks ago

rechecking PR:

on transfer, modal is being shown behind the other modal:

modal being shown behind.webm

buy now and listing are good

Jarsen136 commented 3 weeks ago

@hassnian @prury Thanks for all the comments above. I will close this PR and leave it to the other one who has a better solution.