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.

Sign up for GitHub

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

Jump to bottom

Blur QR-code image when asking permission #305

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Blur QR-code image when asking permission #305

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/popup/modules/UserInterface.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,10 @@ export function handleQrError(error) {
function triggerFileSave(file, filename, requestDownloadPermissions) {
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
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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

"content_security_policy": "default-src 'self'; img-src data:; style-src 'self' https://unpkg.com; script-src 'self' https://unpkg.com",
our current CSP is defined (plus the other manifest files).

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.



Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change

downloadPermissionGranted.then((isAlreadyGranted) => {
let usePermissionWorkaround = false;

Expand All @@ -388,6 +392,7 @@ function triggerFileSave(file, filename, requestDownloadPermissions) {
CommonMessages.showInfo("requestDownloadPermissionForQr");
}


Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change

browser.runtime.sendMessage({
type: COMMUNICATION_MESSAGE_TYPE.SAVE_FILE_AS,
usePermissionWorkaround: usePermissionWorkaround,
Expand All @@ -411,10 +416,13 @@ function triggerFileSave(file, filename, requestDownloadPermissions) {
if (usePermissionWorkaround) {
// if permission result is there, hide info message
CommonMessages.hideInfo();

Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change

Also please don't add unnecessary empty lines here


}

// in case of success there is nothing else to do
// in case of success remove the blur and return
if (permissionGranted) {
qrCode.style.filter = "none";
return;
}

Expand Down
Loading