first try to resolve issue #157 #257
Conversation
There was a problem hiding this comment.
Hi @schmelto,
thanks for your first contribution to this project! 🎉 👍 🏅
I hope you'll like this project and enjoy hacking on it… 😃
This is a start, but surely does not work yet. Or did you check the button and notice any difference?
Here is what needs to be done:
- the settings need to be made translatable (i.e. localize them, we have a module for this), see comment
- you need to define the default value, see comment
- most importantly: you need to read the settings value somewhere (see this module) and then add the logic to actually hide or show the context menu entries.
Also, BTW, it may be a good idea to adjust the title of this PR to explain what you did and not only link to the issue number.
You can also (automatically) let issues close when a PR is merged by adding some "magic" text to your PR body. (Manually linked it now.)
BTW: Next time, try to avoid doing a pull request from the master branch, because you can run into problems when you have a "non-clean" master that does not follow this repo here (i.e. "upstream"). See this article for details. Anyway, this is only a tip for the next time. 😃
| @@ -20,3 +20,5 @@ RandomTips.init(tips).then(() => { | |||
| RandomTips.setContext("options"); | |||
| RandomTips.showRandomTipIfWanted(); | |||
| }); | |||
|
|
|||
| document.getElementById("popupShowContextMenu").checked = true; | |||
There was a problem hiding this comment.
Nope, this is not how we do it in this project. See this answer and this doc (also consider to read the whole one). You have to define the default setting in DefaultSettings.js.
| <li> | ||
| <div class="line"> | ||
| <input class="setting save-on-change" type="checkbox" id="popupShowContextMenu" name="popupShowContextMenu"> | ||
| <label data-i18n="__MSG_optionPopupShowContextMenu__" for="popupIconShowContextMenu">Show context menu item for selected text</label> |
There was a problem hiding this comment.
All __MSG things need localisation. (At least the English one if you speak no other language.) See this answer again and this doc.
|
@schmelto Are you still interested in completing this? If not, that's no problem, just add a notice, so I know what the current status of this PR is. |
|
@rugk sorry I'll give it a try at this weekend |
|
@schmelto Are you still interested in completing this? If not, that's no problem, just add a notice, so I know what the current status of this PR is. |
|
Hey @rugk think this is outdated from me and someone else can pick it up :) |
first try to resolve issue #157