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

Feature: image margins #50

Closed

Conversation

covercash2
Copy link

@covercash2 covercash2 commented Feb 7, 2024

Our users want the ability to add a margin to the embedded image in QR codes.

Added Build::image_gap and changed the argument of Builder::image_size to remove the gap argument so it can be supplied separately.

@covercash2 covercash2 force-pushed the feature/image-margins branch from 56ee174 to 26c3e1d Compare February 7, 2024 21:53
@erwanvivien
Copy link
Owner

erwanvivien commented Feb 9, 2024

Hey @covercash2 thanks for the PR!

There already was the margins, it's the second param in image_size, I don't understand the difference between our outputs, I mean agree that your changes are great, but could you explain please :)

I also see you have update the generated JS bindings
I'd like to know your wasm-bindgen version (wasm-bindgen --version) so I can update the .wasm too

@covercash2
Copy link
Author

covercash2 commented Feb 9, 2024

i didn't see a way to define the gap without defining an image_size which seems to require some complicated downstream algebra 😅
we could definitely go that route, but this seemed simpler, unless i missed something.

this was a fresh install of wasm-bindgen for me:

❯ : wasm-bindgen --version
wasm-bindgen 0.2.91

thanks for the review! 🙏

@erwanvivien
Copy link
Owner

Ahh, I understand the problem!
I'll reveiw a bit more the code, I think we can remove the double computaions

erwanvivien added a commit that referenced this pull request Feb 10, 2024
@erwanvivien
Copy link
Owner

I simplified code in the library too here #51

@covercash2
Copy link
Author

i'll defer to #51 then! 😄

@covercash2 covercash2 closed this Feb 13, 2024
erwanvivien added a commit that referenced this pull request Feb 28, 2024
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