-
-
Notifications
You must be signed in to change notification settings - Fork 438
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 #176
base: master
Are you sure you want to change the base?
Conversation
@feross The standard test is failing on the ci because I'm using |
You have to use |
Hi everyone, I see on my tests that node v18 passes the test while v14 doesn't.
This is reproduced with the window.location.origin, under docker.io/library/node:14 using podman as container. |
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.
The <div id="qr-code"></div>
is output inside a <p>
element literally displaying the tag unrendered.
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.
That's my first review of code, I hope it helps
@@ -186,6 +187,10 @@ function onTorrent (torrent) { | |||
'<a href="' + torrent.torrentFileBlobURL + '" target="_blank" download="' + torrentFileName + '">[Download .torrent]</a>' | |||
) | |||
|
|||
util.log('<div id="qr-code"></div>') |
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.
This should be changed to util.unsafeLog
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.
According to suggestion below we won't need this line
util.log('<div id="qr-code"></div>') |
@@ -186,6 +187,10 @@ function onTorrent (torrent) { | |||
'<a href="' + torrent.torrentFileBlobURL + '" target="_blank" download="' + torrentFileName + '">[Download .torrent]</a>' | |||
) | |||
|
|||
util.log('<div id="qr-code"></div>') | |||
|
|||
util.log(document.getElementById('qr-code').appendChild(kjua({text: location.origin + '/#' + torrent.infoHash, crisp: true}))) |
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.
I am really unsure if you need to encapsulate to document.getElementById
into util.log
. Simply leaving it out produces the same result cause you are already selecting the "qr-code" id div and putting stuff there.
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.
This is a pretty old PR request, happy to make the changes to it. I do remember the templating being a little weird when trying to reference document outside of util.log because (at least at the time), document would get seen as undefined by the tests.
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.
Actually, for multiple "uploads" in the implementation you made you putting the qr code on the original first drawn div.
You could replace both lines with
util.log(document.getElementById('qr-code').appendChild(kjua({text: location.origin + '/#' + torrent.infoHash, crisp: true}))) | |
util.unsafeLog(kjua({ | |
text: window.location.origin + '/#' + torrent.infoHash, | |
crisp: true | |
}).outerHTML) |
This will output "in place" each time you make an upload,underneath the buttons, above the content preview (if any).
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.
@joshterrill I feel like the QR codes are a feature to help adoption and provide ease of use. I think it's important, furthermore, it's not resolved in other PRs. Personally, I just stroll through repositories I like and try to help in whichever way I can. So, if you still wish to make this contribution, here is my review 😃 I guess @feross should also agree to push this, but I saw it was stuck because of a test failing (standard). The error I post I was getting is about wrong permissions on podman
in my attempt to replicate the "testing" method from the repo (node 14, ubuntu latest). I am not affiliated with the development team, so my opinion is as good as that can be. Cheers!
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.
Yeah I'm good to make these changes, would love to see this implemented as well. You'll see in this PR that I asked a question to resolve the issue with the standard tests not recognizing javascript window objects in the node environment, and it went unanswered for two years. So I hope that we can resolve this! I also see some of the suggestions you made (like util.unsafelog
) were things that didn't even exist in the repo (as far as I can tell) at the time that this code was originally written, so that's cool. I'll take a look at this today and get back to you. Thanks.
This is in place of #104
And uses the
kjua
library that was recommended in #94