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

Fix qrbox border placement #555

Merged
merged 2 commits into from
Oct 24, 2022

Conversation

allanbrault
Copy link
Contributor

First commit fix an issue where the wrapping element of the video element added some whitespaces and made the qrbox overflow. ref: https://stackoverflow.com/a/36397260
Screenshot 2022-10-13 at 11 12 12
Screenshot 2022-10-13 at 11 13 25

Second commit now fix an issue raised by the first fix, the qrbox borders are based on the size of the viewFinder size, problem is when the viewFinder height has a floating point the position of the 4 bottom borders are now based on half of a rounder floating number.

This PR also had the issue with 5px too much, this is because the 4 bottom borders where placed in relation to the viewFinder area + adding 5px to compensate the whitespace from the video element.

Final result:
Screenshot 2022-10-13 at 11 35 08

A video element behave like an inline element, which mean it honours
whitespace around. When in a div with whitespace it adds empty spaces.

ref: https://stackoverflow.com/a/36397260
Using the bottom allow the 4 boxes on the bottom of the qrbox to be
pixel perfect on the bottom of the qrbox area, instead of relying on the
qrbox size that can be with a floating point.
Copy link
Owner

@mebjas mebjas 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 the changes!

@mebjas mebjas merged commit f9f8086 into mebjas:master Oct 24, 2022
@mebjas mebjas mentioned this pull request Oct 24, 2022
@allanbrault allanbrault deleted the fix_qrbox_border_placement branch February 20, 2023 09:47
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.

2 participants