Blur QR-code image when asking permission #305
Conversation
|
I was wondering what the status of this pull request is |
rugk
left a comment
rugk
left a comment
There was a problem hiding this comment.
Hi @serkanalgur ,
first of all, thanks for your first contribution to this project! 🎉 👍 🏅
I hope you'll like this project and enjoy hacking on it… 😃
And sorry for the delay, I'm currently quite busy with other things so sorry for not handling this earlier.
Note you can (automatically) let issues close when a PR is merged by adding some "magic" text to your PR body. (Manually linked it now.)
| // Blur QR code so that it is clear that user did not give permission yet | ||
| qrCode.style.filter = "blur(5px)"; // add the blur effect | ||
|
|
||
|
|
| const downloadPermissionGranted = browser.permissions.contains(DOWNLOAD_PERMISSION); | ||
|
|
||
| // Blur QR code so that it is clear that user did not give permission yet | ||
| qrCode.style.filter = "blur(5px)"; // add the blur effect |
There was a problem hiding this comment.
At least the comment on this line is unnecessary IMHO. Its okay you could leave it, but have a look at the clean code prinicples. One big takeaway is that the code itself should be self-explanatory and structured in way that you actually don't need to comment everything. If the code itself shows what is done there is no advantage in adding a comment that says the same again. Instead such comments can even be bad as they “rot” and may become outdated.
| CommonMessages.showInfo("requestDownloadPermissionForQr"); | ||
| } | ||
|
|
||
|
|
| const downloadPermissionGranted = browser.permissions.contains(DOWNLOAD_PERMISSION); | ||
|
|
||
| // Blur QR code so that it is clear that user did not give permission yet | ||
| qrCode.style.filter = "blur(5px)"; // add the blur effect |
There was a problem hiding this comment.
Did you test this actually? If so its a great idea to include a screenshot in the PR body (you can just paste it in there) to demonstrate that and to show how it looks like.
Also we have a quite strict CSP (content security policy, so I question whether adding CSS like that inline should even work). See https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/Content_Security_Policy#default_content_security_policy for the default one Mozilla has for extensions and https://extensionworkshop.com/documentation/develop/build-a-secure-extension/ for more context on secure extensions.
Here
offline-qr-code/src/manifest.json
Line 43 in 04a6089
The alternative is easy, just using/adding a class. Another advantage is you can name the class semantically good, so that it is clear what it does by its name and so again you have clear code and don't need a comment.
You cab find a lot of resources about CSS naming: Guides by example and mdn with general tips. The important thing is just consistency, however, and that your variable names are explanatory and explain semantics.
| if (usePermissionWorkaround) { | ||
| // if permission result is there, hide info message | ||
| CommonMessages.hideInfo(); | ||
|
|
There was a problem hiding this comment.
Also please don't add unnecessary empty lines here
2 participants
This pull request should fix issue 148.
The solution is a very simple one.
It adds a blur effect to a qrCode element by setting its style.filter property to "blur(5px)". This is done to indicate to the user that they have not granted the download permission yet.
Finally, the function checks if the permission is granted. If it is, it removes the blur effect from the qrCode element and returns from the function. Otherwise, it does nothing.
Please let me know if there is anything to add to this issue! Would love to contribute to this project.