Skip to content
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

655: replace qr_code_scanner with mobile_scanner #679

Merged
merged 7 commits into from
Jan 11, 2023

Conversation

sarahsporck
Copy link
Contributor

No description provided.

@sarahsporck sarahsporck linked an issue Dec 19, 2022 that may be closed by this pull request
@sarahsporck
Copy link
Contributor Author

@maxammann as you wished :)

@sarahsporck sarahsporck force-pushed the 655-replace-qr-code-scanner-with-mobile-scanner branch from 3a19a8d to 95b153c Compare December 21, 2022 14:00
@sarahsporck sarahsporck force-pushed the 655-replace-qr-code-scanner-with-mobile-scanner branch from 95b153c to 77ff0f6 Compare January 9, 2023 12:13
@sarahsporck sarahsporck force-pushed the 655-replace-qr-code-scanner-with-mobile-scanner branch from 49261f9 to 521e895 Compare January 9, 2023 13:05
@sarahsporck sarahsporck marked this pull request as ready for review January 9, 2023 13:05
@sarahsporck sarahsporck requested a review from maxammann as a code owner January 9, 2023 13:05
@sarahsporck sarahsporck changed the title 655: replace qr_code_scanner with mobile_scanner (WIP) 655: replace qr_code_scanner with mobile_scanner Jan 9, 2023
Copy link
Member

@maxammann maxammann left a comment

Choose a reason for hiding this comment

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

Testing now on iOS and Android

@maxammann
Copy link
Member

And I suspect this does not compile on iOS right now, because it is stuck in the check job

@maxammann
Copy link
Member

Tested on iOS (release and debug)

@maxammann
Copy link
Member

I also fixed linting

Copy link
Member

@michael-markl michael-markl left a comment

Choose a reason for hiding this comment

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

(Only code review)

@sarahsporck
Copy link
Contributor Author

If I understand correctly, this is just a visual overlay to allow scanning qr codes inside of the overlay? We could just ignore that for now, until the feature has officially landed in mobile_scanner. (So we could just remove this overlay shape and the _calculateScanArea function in qr_code_scanner_controls.dart)

However, if it works without problems like this, we can also leave it like this I think :) (And maybe refactor once the feature has landed)

for me it works without problems. And it seems it will take a little bit until integration, therefore I'd keep the solution as it is.

Tested on Android again. Works fine.
@maxammann do you want to test it for ios once more please?

@maxammann
Copy link
Member

Tested on Android again. Works fine.
@maxammann do you want to test it for ios once more please?

No need for that if it works on Android without that crash merge :)

@maxammann maxammann merged commit 245a05d into whitelabel Jan 11, 2023
@maxammann maxammann deleted the 655-replace-qr-code-scanner-with-mobile-scanner branch January 11, 2023 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace qr_code_scanner with mobile_scanner
3 participants