search

kazuhikoarase / qrcode-generator

QR Code Generator implementation in JavaScript, Java and more.
https://kazuhikoarase.github.io/qrcode-generator/js/demo/
MIT License
2.1k stars 677 forks source link

scalable svg #37

Closed mix3d closed 5 years ago

mix3d commented 7 years ago

by using the viewBox parameter instead of directly setting width and height, the svg can scale cleanly to any size display. This backwards compatible change would default to the same behavior, but also either set a defined pixel size of the users choice, or 'auto'/-1 to not include the width/height, making the svg scale to 100% of the parent width.

ahwayakchih commented 5 years ago

@mix3d what if, instead of adding third argument (displaySize), SVG renderer would just check if cellSize is anything else than 0? If it's 0, then it would set cell size to default, but render scalable SVG. Otherwise, it would render as usual. I mean, there's not much point in changing cellSize when result is fully scalable, right? Edit: Problem is that it could break older code, where someone was calling without arguments but counted on width and height in the output. Actually, if -1 was used for "scalable" instead of 0 it would stay backward compatible. It would just have to be properly documented.

mix3d commented 5 years ago

I'd rather have it be semantically intuitive, and use 'auto' instead of -1, but would be happy to make that change

ahwayakchih commented 5 years ago

Yeah, that could work too. Or maybe some symbol/const exported by library? So internally it would stay numeric type, but user could call something like:

var svg = qr.createSvgTag(qr.SCALABLE);
ahwayakchih commented 5 years ago

Of course that's just my point of view, and i'm not the one who can make final decision on this :).

@kazuhikoarase any thoughts on this?

kazuhikoarase commented 5 years ago

I merged it with develop branch and tried to refactor. Will this help you?

https://github.com/kazuhikoarase/qrcode-generator/commit/633a0d3b5de6f6fa90cd2c6fffc0035621b7fa91

ahwayakchih commented 5 years ago

Yes, having options object gives us some space for any other extensions in the future.

Thanks!

mix3d commented 5 years ago

Downside is that it's a breaking change for existing users, but a much cleaner approachb moving forward

On Fri, Feb 15, 2019, 4:17 AM Marcin Konicki notifications@github.com wrote:

Yes, having options object gives us some space for any other extensions in the future.

Thanks!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kazuhikoarase/qrcode-generator/pull/37#issuecomment-463964672, or mute the thread https://github.com/notifications/unsubscribe-auth/ABZ-WcUG1KOYpqMz4wuVAsJXJd7gP8xaks5vNnsZgaJpZM4NipNJ .

ahwayakchih commented 5 years ago

@mix3d how is the change breaking? options object was not supported before, so only new code will use it. "Old" cellSize, margin signature is still supported as it was before AFAIK.

mix3d commented 5 years ago

Ah, I missed the part in the diff where the function was overloaded. Never mind!

On Sat, Feb 16, 2019, 1:38 PM Marcin Konicki notifications@github.com wrote:

@mix3d https://github.com/mix3d how is the change breaking? options object was not supported before, so only new code will use it. "Old" cellSize, margin signature is still supported as it was before AFAIK.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kazuhikoarase/qrcode-generator/pull/37#issuecomment-464370656, or mute the thread https://github.com/notifications/unsubscribe-auth/ABZ-WS49fkla0-epBfJ3OC3tE4Z8fS-Wks5vOFAIgaJpZM4NipNJ .