Skip to content
This repository has been archived by the owner on Feb 3, 2025. It is now read-only.

Turn off Qr scanner after pasting from clipboard #203

Closed
wants to merge 7 commits into from
Closed

Turn off Qr scanner after pasting from clipboard #203

wants to merge 7 commits into from

Conversation

benalleng
Copy link
Collaborator

As noted in issue MutinyWallet/mutiny-web-poc#12 the qr scanner doesn't properly turn off when pasting an address from a clipboard and only on a successful qr scan.
Upon troubleshooting this problem i discovered it seemed to do with the fact that the input box and the QrCodeSanner component were separate and joined together in the send route. The qr scanner is only looking for valid data scanned from the qr code and not valid data typed in the input field. At first I attempted to import the qrCodeRef to find the existing qr scan and then kill it upon calling the handleContinue() function of the send route with,
qrCodeRef.current?.stop()
however, I was not able to get react to successfully identify the qr scanner that was started in the separate component so I instead decide to build matching logic for the input field inside of the QrCodeScanner component to ensure that when the continue button is pressed it can find and kill the existing qr scanner.
Lastly I added a small amount of styling because that qr scanner is too damn BIG!

I am happy that it works but would have liked to simplify the logic and not double up the code 😅 just thought I'd give a hand at fixing this bug that i thought could be fixed more easily.
If there is a way to find and kill the Qr scanner from outside of the component this code could be much more simplified.

@benalleng benalleng marked this pull request as draft December 12, 2022 02:16
@benalleng
Copy link
Collaborator Author

benalleng commented Dec 12, 2022

Upon testing this more I realized that this really isn't a solution that as the closing X in the top right should also close the qr scanner but seeing as it is also in the route I am not sure how to call the qrCodeRef as it is created in the qrCodeScanner component without also moving it into the qrCodeScanner component.
While i continue to work on this i welcome any feedback or help on allowing it to be callable in the Send route so that the qr scanner closes both on pressing the continue button and the close button.
Feel free to reject this merge as it really isn't up to par, perhaps we can continue to discuss how to best solve this in MutinyWallet/mutiny-web-poc#12

Copy link
Contributor

@futurepaul futurepaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for digging into this, def a hairy part of the codebase

I think moving the ref state into Send.tsx and passing it as a prop to QrCodeScanner solves your problem, along with giving an optional callback. let me know if that won't work for some reason

@@ -118,3 +118,16 @@ dd {
*::-webkit-scrollbar {
display: none;
}

.qrContainer {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for one-off styling I prefer inline tailwind when possible

@@ -16,6 +36,82 @@ const QrCodeDetectorComponent = ({
const [errorMessage, setErrorMessage] = useState("")
const [cameraReady, setCameraReady] = useState<boolean>(false)
const qrCodeRef = useRef<Html5Qrcode | null>(null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the move here is to raise this qrCodeRef up into Send.tsx, rather than lowering the navigate functions into here. that way Send.tsx can call anything it needs for cleanup on the ref.

ideally the QrCodeScanner stays rather dumb and generic (ideally it should handle other scenarios, like imagine it's being used for lnurl auth... it detects a valid qr code and then runs whatever callbacks have been passed to it from its parent.)

@@ -129,10 +127,6 @@ function Send() {
</header>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it would help if the component would accept some onClose callback that overrides the default behavior of navigating to root, that way we can pass cleanup logic when necessary

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

referring to the <Close /> component

@futurepaul
Copy link
Contributor

okay #221 should hopefully unblock you on this, let me know if you have any qs!

@benalleng
Copy link
Collaborator Author

Going to give this another crack this weekend!

@benalleng benalleng closed this Dec 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants