feat: Control return to confirm action #10306

Jarsen136 · 2024-05-15T13:44:50Z

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

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

PR Type

Feature

Needs Design check

@exezbcz please review

Needs QA check

@kodadot/qa-guild please review

Context

Closes Control return to confirm action #10213

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

My DOT address: Payout

Screenshot 📸

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

netlify · 2024-05-15T13:45:05Z

✅ Deploy Preview for koda-canary ready!

Name	Link
🔨 Latest commit	`806c624`
🔍 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... Use your smartphone camera to open QR code link.

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

hassnian

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?

prury · 2024-05-16T17:51:35Z

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 · 2024-05-16T21:03:54Z

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 · 2024-05-17T12:28:12Z

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 · 2024-05-19T23:13:21Z

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.

hassnian · 2024-05-20T12:10:15Z

modal animation is gone

this pr

CleanShot.2024-05-20.at.17.03.43.mp4

canary

CleanShot.2024-05-20.at.17.04.16.mp4

hassnian · 2024-05-20T12:13:20Z

libs/ui/src/components/NeoModal/NeoModal.vue

@@ -1,5 +1,5 @@
 <template>
-  <teleport to="body" :disabled="!appendToBody">
+  <teleport v-if="isModalActive" to="body" :disabled="!appendToBody">


can we do a local solution for each modal

Why apply it for each modal? It's kind of a bug from OModal that the DOM of modal will not be removed after closing the modal.

pls check

@daiagi what do you think.

Oh sorry, I miss the comment.

The missing animation was zoom-out

It's a trade-off. Whether we keep the animation as currently we have, or solve the memory leak of modal.
I would fix the memory leak issue.

Here is the screenshot that, I closed the modal while the DOM of modal was still there.

hassnian · 2024-05-20T12:18:12Z

components/common/autoTeleport/AutoTeleportActionButton.vue

+useKeyboardKeys({
+  onPressControlEnter: handleSubmit,
+})


enabled if action button is visible

It's not needed, because the keyboard event would be listened to only when the action button component is mounted.

I was suggesting this if we removed the teleport v-if

Got it. The trickier thing is that the action button is always visible and not be unmounted, due to the memory leak of the modal.... #10306 (comment)

hassnian · 2024-05-20T12:19:37Z

composables/useKeyboardKeys.ts

@@ -0,0 +1,11 @@
+import { useMagicKeys } from '@vueuse/core'
+
+export function useKeyboardKeys({ onPressControlEnter }) {


Suggested change

export function useKeyboardKeys({ onPressControlEnter }) {

export function useKeyboardKeys({ onPressControlEnter , enabled }) {

hassnian · 2024-05-20T12:20:44Z

components/transfer/TransferConfirmModal.vue

+useKeyboardKeys({
+  onPressControlEnter: confirmTransfer,
+})


enabled if isModalActive

the same reason as the above comment

codeclimate · 2024-05-22T15:00:36Z

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

View more on Code Climate.

sonarcloud · 2024-05-22T15:01:22Z

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 · 2024-05-22T17:15:01Z

rechecking PR:

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

modal.being.shown.behind.webm

still working for try again on teleport
create nft/collection: modal opens behind

buy now and listing are good

Jarsen136 · 2024-05-22T19:13:29Z

@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.

feat: Control return to confirm action

46c871e

Jarsen136 marked this pull request as ready for review May 15, 2024 13:44

Jarsen136 requested a review from a team as a code owner May 15, 2024 13:44

Jarsen136 requested review from daiagi and hassnian and removed request for a team May 15, 2024 13:44

hassnian reviewed May 16, 2024

View reviewed changes

prury added the S-changes-requested-🤞 PR is almost good to go, just some fine tunning label May 16, 2024

prury and others added 3 commits May 16, 2024 14:52

Merge branch 'main' into issue-10213

1ab472a

refactor: useMagicKeys

6964f2b

fix: memory leak for NeoModal

3faf956

Jarsen136 requested a review from hassnian May 16, 2024 21:04

Jarsen136 added 2 commits May 20, 2024 00:42

Merge branch 'main' into issue-10213

e8a8832

fix: transfer modal confirm keyboard

23f5674

hassnian reviewed May 20, 2024

View reviewed changes

Merge branch 'main' into issue-10213

806c624

Jarsen136 closed this May 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Control return to confirm action #10306

feat: Control return to confirm action #10306

Jarsen136 commented May 15, 2024

netlify bot commented May 15, 2024 •

edited

hassnian left a comment •

edited

prury commented May 16, 2024

Jarsen136 commented May 16, 2024

prury commented May 17, 2024

Jarsen136 commented May 19, 2024

hassnian commented May 20, 2024

hassnian May 20, 2024

Jarsen136 May 20, 2024

hassnian May 20, 2024

Jarsen136 May 20, 2024

hassnian May 20, 2024 •

edited

Jarsen136 May 20, 2024

hassnian May 20, 2024 •

edited

Jarsen136 May 20, 2024

hassnian May 20, 2024 •

edited

hassnian May 20, 2024

Jarsen136 May 20, 2024

codeclimate bot commented May 22, 2024

sonarcloud bot commented May 22, 2024

prury commented May 22, 2024

Jarsen136 commented May 22, 2024 •

edited

		@@ -0,0 +1,11 @@
		import { useMagicKeys } from '@vueuse/core'

		export function useKeyboardKeys({ onPressControlEnter }) {

feat: Control return to confirm action #10306

feat: Control return to confirm action #10306

Conversation

Jarsen136 commented May 15, 2024

PR Type

Needs Design check

Needs QA check

Context

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

Screenshot 📸

netlify bot commented May 15, 2024 • edited

✅ Deploy Preview for koda-canary ready!

hassnian left a comment • edited

Choose a reason for hiding this comment

prury commented May 16, 2024

Jarsen136 commented May 16, 2024

prury commented May 17, 2024

Jarsen136 commented May 19, 2024

hassnian commented May 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hassnian May 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hassnian May 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hassnian May 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeclimate bot commented May 22, 2024

sonarcloud bot commented May 22, 2024

Quality Gate passed

prury commented May 22, 2024

Jarsen136 commented May 22, 2024 • edited

netlify bot commented May 15, 2024 •

edited

hassnian left a comment •

edited

hassnian May 20, 2024 •

edited

hassnian May 20, 2024 •

edited

hassnian May 20, 2024 •

edited

Jarsen136 commented May 22, 2024 •

edited