-
Notifications
You must be signed in to change notification settings - Fork 1
Bug fixes #14
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14 +/- ##
========================================
- Coverage 0.84% 0.83% -0.02%
========================================
Files 3 3
Lines 118 120 +2
Branches 19 20 +1
========================================
Hits 1 1
- Misses 117 119 +2
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
components/BarcodeScanner.vue
Outdated
| const height = window.innerHeight; | ||
| // must parse float aspect ratio value | ||
| const landscapeAspectRatio = parseFloat((width / height).toFixed(3)); | ||
| const portraitAspectRatio = parseFloat((height / width).toFixed(3)); |
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.
Can we compute the aspect ratio just once here using something like:
// determine aspect ratio based on portrait/landscape orientation
const aspectRatio = parseFloat(
isPortrait() ? height / width : width / height).toFixed(3);And helper in helpers section below:
function _isPortrait() {
return window.matchMedia('(orientation: portrait)').matches;
}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.
So a few things I've found that are a little strange that I'm trying to work around...
The desktop is in portrait mode, but it is not portrait. So for it to fill the whole screen on desktop and mobile, we will want to follow the _isPortrait function you wrote above unless it is desktop. If we are rendering the camera on desktop we want it to act like it is landscape. So we are back to using $q.platform.is.desktop
components/BarcodeScanner.vue
Outdated
| const aspectRatio = computed(() => { | ||
| const width = window.innerWidth; | ||
| const height = window.innerHeight; | ||
| const isDesktop = !$q.platform.is.desktop; |
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.
Let's move the desktop check into the _isPortrait() helper and just say, for now, that desktop is considered never in portrait mode. We'll want to figure out what's broken there in the future.
Co-authored-by: Dave Longley <[email protected]>
Resolves - bug fix
What kind of change does this PR introduce?
What is the current behavior?
What is the new behavior?
Does this PR introduce a breaking change?
How has this been tested?
Screenshots: