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

Added qr code generation #104

Closed
wants to merge 4 commits into from
Closed

Added qr code generation #104

wants to merge 4 commits into from

Conversation

joshterrill
Copy link
Contributor

Related to #94, added qr code generation upon torrent file selection. Maybe we want to make the QR code larger? Or position it somewhere else? I figured this was fine for right now.

@@ -166,6 +167,10 @@ function onTorrent (torrent) {
'<a href="' + torrent.magnetURI + '" target="_blank">[Magnet URI]</a> ' +
'<a href="' + torrent.torrentFileBlobURL + '" target="_blank" download="' + torrentFileName + '">[Download .torrent]</a>'
)

util.log(
'QR Code: <img class="qr" src="data:image/png;base64,' + qr.imageSync(location.href + '#' + torrent.infoHash, { type: 'png' }).toString('base64') + '">'
Copy link
Member

Choose a reason for hiding this comment

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

Small bug: If there's already a hash in the URL, this will add a second one.

@feross
Copy link
Member

feross commented Mar 19, 2016

You don't need to commit the qrcode.min.js file, since you're already using require to include the package.

@feross
Copy link
Member

feross commented Mar 19, 2016

One thing that's a bit of a bummer is that this single dependency almost doubles the size of the JS bundle.

cat static/bundle.js | gzip | wc -c
  144612
browserify -r node-qr-image | gzip | wc -c
  103611

@feross
Copy link
Member

feross commented Mar 19, 2016

Other than my comments, this looks good!

If you know another dependency that's a bit lighter weight than this, let's swap it in.

@joshterrill
Copy link
Contributor Author

Actually that js file shouldn't even be there. My bad. That was for another
one I was testing out. And yeah ill look for another dependency... most of
the other ones I looked at required some sort of C libraries that I didn't
have installed so I figured it would be bad to use those. But I will still
look around.
On Mar 18, 2016 11:26 PM, "Feross Aboukhadijeh" [email protected]
wrote:

Other than my comments, this looks good!

If you know another dependency that's a bit lighter weight than this,
let's swap it in.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/feross/instant.io/pull/104#issuecomment-198647711

@feross
Copy link
Member

feross commented Mar 19, 2016

No problem if you can't find one. We can still merge this. But there's no way that QR generation can require that much code.

@feross
Copy link
Member

feross commented Mar 19, 2016

It looks like most of the size of node-qr-image comes from bundling a zlib library. We should just find a library that uses canvas to draw the image. You can even get a PNG from a canvas without manually making the PNG yourself ;)

@joshterrill
Copy link
Contributor Author

Woops, forgot this was here. I'll take care of this and fix what needs to be fixed this week.

@feross
Copy link
Member

feross commented Jan 21, 2017

Hey @joshterrill, no pressure, just wondering if you plan to finish up this PR? If not, no worries. Just let me know.

@joshterrill
Copy link
Contributor Author

Sure I can do it. What do you think about using something like this? https://github.com/alexeyten/qr-image

@feross
Copy link
Member

feross commented Jan 25, 2017

Looks like a good library to me.

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