-
Notifications
You must be signed in to change notification settings - Fork 127
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 for removing context menu item #157
Comments
You know that one only sees the context menu item if text is selected and/or if you click on a link? So it's at least not shown for all cases. That said, I would be fine with such a checkbox (enabled by default, as you said). (PRs appreciated…) I am not fine with automatically disabling/modifying it when another option is changed though. The correlation between the settings may not obvious to the user and there are valid use-cases for having both settings enabled. |
Hi there, I would like to work on this issue. The task as i understand it includes the following steps:
As a beginner i would love to get some feedback: |
Hi @shanirub ,
|
|
Hi, I noticed this hasn't been updated since July 22. May I work on the issue? |
@shanirub already did some work on this, as it seems. So let's wait for a reply here. So assigning @shanirub for getting a reply whether @c3ho can/should take over this change or not? |
Hope to see this feature, because Firefox default access key for Inspect Element is also Q. Alternatively allow changing the access key for this addon to R |
For simplicity reasons, we'd only want to have one fixed and value and it should be a word that is included in the context menu text. In any case, however, your underlying issue seems to be valid/a good point, so please open a new issue for that. (As it may be fixed independently to this one, as you've noticed by yourself.) |
@shanirub If you need any pointers/help or have questions, feel free to ask. |
Thank you, that would be great. I could use some help+pointers. |
Well you've already collected some things to do and you already did something. I've also pointed you to the modules that are documented on how they work. As far as I see you already have the setting, you just need to read the setting in the module (done with https://github.com/TinyWebEx/AddonSettings) and apply it. |
@shanirub Are you still interested in it? If not, that's no problem, just add a notice, so I know what the current status of this issue is. |
Hey there, is this one an easy issue for my a looking for the first contribution? |
Hi @schmelto sorry for the delay, I was busy. Yeah, feel free to take this on. It should be easy as it is just about adding a settings options and the logic behind this option.
Yes, mostly. Of course you HTML/CSS knowledge may be useful too. (though you likely don't need CSS knowledge for this task) Most knowledge you'll (need to/will) learn is about the WebExtensions API. See https://developer.mozilla.org/docs/Mozilla/Add-ons/WebExtensions/user_interface/Context_menu_items for a starting point. If you have any questions, feel free to ask. |
Also sorry to the others who I had to unassign/delay from this issue, I'll leave this to @schmelto for now as @schmelto was the last one to comment here and show interest in implementing this feature. |
Hey @rugk, I managed to add a checkbox into the options.html And add this at the top after the Colored Icon:
Kind regards |
Well… this can always be changed later, but I'd place it below the heading "Add-on behaviour".
Yes, technically you always need to specify a default. |
Maybe you can check on my first try to resolve this issue :) thx |
Any follow-up gents? |
@schmelto has closed their pull request, so if anyone wants to try it and is still interested, this is ready to be taken up by somebody. |
Hi there. I'd like to take the issue if it's currently unassigned and still a valid issue |
I've managed to integrate an option for disabling the context menu, however I haven't been able to make the context menu appear at all. Was hoping I could get some help figuring out what I am doing wrong. I've attached the console log from trying to get it to work. And here's the changes I've made so far |
@starborne-nova From having a quick look the code changes all looks fine? Did you make sure to clone the repo with submodules or check them out afterwards? In any case, I'd appreciate a PR, so the option can be integrated. |
\assign |
Okay and thanks @deepak-158, I'll assign you. Everyone contributing to this though please take care not to do work that has already be done: From previous contributors the PR #257 looks quite far/promising and I've already commented/reviewed missing things there. You can just…
You may need to change the origin of the branches or so or pull/push it to a different one while doing so, so the changes you want are in there. You can always open a draft PR though to see what's going on. |
Installing various add-ons can easily clutter up the context menu, even if some items are just present when text is selected.
Please add a checkbox as:
[x] Show context menu item for selected text
. You can keep the current default.I think, when the following already present checkbox is checked:[x] Automatically use the text selected on the website
, the new checkbox should be disabled and unchecked.The text was updated successfully, but these errors were encountered: