-
Notifications
You must be signed in to change notification settings - Fork 129
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
Add settings checkbox #276
Conversation
Hi @starborne-nova, Sorry for missing the PR so long, I was busy and totally missed this here. |
You can (automatically) let issues close when a PR is merged by adding some "magic" text to your PR body. (Manually linked it now.) |
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.
Generally looks quite good, but there are some minor things to change.
}); | ||
} | ||
const menuDisabled = AddonSettings.get("disableContextMenu") | ||
|
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 empty line is not helpful IMHO ;)
browser.menus.onShown.addListener(menuShown); | ||
}); | ||
} | ||
} |
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.
} | |
} |
Please mind to keep the code-format in tact. Some IDEs can autoformat the files.
const menuDisabled = AddonSettings.get("disableContextMenu") | ||
|
||
|
||
if(menuDisabled === false){ |
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.
if(menuDisabled === false){ | |
if (menuDisabled === false) { |
another codeormat thing to keep spaces here
@@ -0,0 +1,207 @@ | |||
21:42:28.783 Offline QR code [WARN] could not get managed options Error: Managed storage manifest not found Logger.js:76:21 |
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 guess this log file was accidentally committed and you may delete it 🙃
<li> | ||
<div class="line"> | ||
<input class="setting save-on-change" type="checkbox" id="disableContextMenu" name="disableContextMenu"> | ||
<label data-i18n="__MSG_optionDisableContextMenu__" for="disableContextMenu">Disable the context menu item for selected text</label> |
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.
Great you added that option and i18n entry. However please also provide the translation string in the message.json
file, so people can translate it. It is enough to provide the English translation, of course, so you can just copy and paste the string from here.
This ensures, that text snippet is ready for translation and can be localized.
For more information, please see the MDN guide on how to localize WebExtensions and our internationalization (i18n) guide.
Regarding issue #157
-Added setting to disable right-click context menu items for the addon
-updated Contributors.md