Add context-menu item for generating QR code for the current page (#321) #336

mahmoudhusam · 2024-10-14T11:44:38Z

This PR adds a context-menu item to generate a QR code for the current page URL. The functionality is implemented in the ContextMenu.js module, and a new message string was added to messages.json for localization.
Resolves issue #321.

…gk#321)

src/background/modules/ContextMenu.js

src/_locales/en/messages.json

Co-authored-by: rugk <[email protected]>

rugk

Hi @mahmoudhusam,
first of all, thanks for your first contribution to this project! 🎉 👍 🏅
I hope you'll like this project and enjoy hacking on it… 😃

It generally looks good, but I have some pointers to change:

First of all, would you have a screenshot or even screencast to show (how) it works?
Secondly, I see you may need to add removing that menu entry to the menuShown function like the others? I am not sure why, but maybe because it is re-rendered… and may not be shown every time? I am unsure, and unfortunately the JSDoc is also not helpful there (blame on me, but feel free to improve it)
Generally, as explained in the issue before (Add context-menu item for generating a QR code for _the current page_ #321 (comment)) the feature needs to be configurable. That is as far as I see still missing. See Add settings checkbox for removing context menu item #157 (comment) and FYI for some guides/pointers on how to introduce such a setting.

rugk · 2024-10-15T22:43:39Z

src/background/modules/ContextMenu.js

+            // Send the current page URL to the popup
+            sendQrCodeText(event.pageUrl);


AFAIK/IIRC you don't need to do that explicitly, as it is the default behaviour. Though settings can obviously change that and if you e.g. always use the text selection then you have two menu entries that would do the same, so IMHO, yeah… makes sense and this is fine.

Could you just test this use case? Aka enable the setting to use the text-selection by default and then use this menu entry? Does it work as expected?

Also maybe adding this here as an explanation seems good:

Suggested change

// Send the current page URL to the popup

sendQrCodeText(event.pageUrl);

// Send the current page URL to the popup explicitly to overwrite any setting that may use a different string

sendQrCodeText(event.pageUrl);

…gk#321)

mahmoudhusam · 2024-10-20T10:49:10Z

Hi @rugk,

Thank you so much for your guidance and for helping me through my first contribution to open-source! I've learned a lot from this project, and your feedback has been incredibly valuable.

I've implemented the requested changes:

Added a screencast demonstrating the new feature in action (attached below).
Please let me know if there's anything else you'd like me to adjust. I'm more than happy to make further improvements!

Thanks again for your support.

rugk

Great! Did you test it and everything worked?

I've left some comments for spots, where things need to be improved, but we're close though…

rugk · 2024-10-22T22:39:58Z

src/options/modules/CustomOptionTriggers.js

+ */
+document.getElementById('contextMenuEnabled').addEventListener('change', async (event) => {
+    const isChecked = event.target.checked;
+    await browser.storage.local.set({ contextMenuEnabled: isChecked });


Please don't handle option saving manually.

Also, it's already saved by the library, so no need to save it again… AutomaticSettings.Trigger below handles it all, so this whole custom handling/element registration should be removed…

rugk · 2024-10-22T22:42:35Z

src/options/options.html

@@ -64,6 +64,15 @@ <h1 data-i18n="__MSG_titleGeneral__">Add-on</h1>
 							<span class="helper-text" data-i18n="__MSG_optionPopupIconColoredDescr__">Shows a colored icon instead of the black/white icon in the toolbar.</span>
 						</div>
 					</li>
+					<li>
+						<div class="line">
+							<input class="setting save-on-change" type="checkbox" id="contextMenuEnabled" name="contextMenuEnabled">


Great! Could you add the required i18n attributes to enable localisation? Aka internationalize this part here?

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.

Also cf the rest of the options HTML of course.

rugk · 2024-10-22T22:45:20Z

src/background/modules/ContextMenu.js

@@ -61,6 +61,17 @@ function createItems() {
        contexts: ["page"]
    });

+    browser.runtime.onMessage.addListener((message) => {


Okay, generally thougj let's move that into the init functions below, don't we? Otherwise it could get re-registered and triggered multiple times when createItems() is called?

src/options/options.html

…ributes, improved UX for context menu setting, and moved event listener

mahmoudhusam · 2024-10-23T13:41:25Z

Hi @rugk,

Thanks for the detailed feedback! I've made the following adjustments based on your suggestions:

Removed the manual saving of the option in CustomOptionTriggers.js, as it’s already handled by AutomaticSettings.Trigger.
Added the necessary i18n attributes for localization in options.html and updated messages.json with the corresponding strings.
Moved the browser.runtime.onMessage.addListener to the init function in ContextMenu.js to prevent multiple registrations.
Clarified the label and helper text for the context menu setting to avoid confusion and improve UX.

Looking forward to your thoughts!

rugk

Thanks! Code-wise, this looks like it should work.

rugk · 2024-10-23T14:16:18Z

Okay, tried it out and pushed the German translations.

Also noticed we could change the behavior to also show this entry when some other context menu item is shown, so the user could decide which one to use.

Like e.g. if you select text:

…or click a link:

…or even do both:

The issue is, Firefox automatically makes it a sub-context menu as you can see – and it does not look as nice, IMHO.

Code-wise, the change is simple, but changes a lot:

diff --git a/src/background/modules/ContextMenu.js b/src/background/modules/ContextMenu.js
index f60034e..910560a 100644
--- a/src/background/modules/ContextMenu.js
+++ b/src/background/modules/ContextMenu.js
@@ -58,7 +58,7 @@ function createItems() {
 
     const pageMenu = createMenu("contextMenuItemConvertPageURL", {
         id: CONVERT_PAGE_URL,
-        contexts: ["page"]
+        contexts: ["page", "link", "selection"]
     });
 
     browser.menus.refresh();

I've tested it and it would work, but by testing it, I think, we better leave it as it is, because then only one context menu item is shown when users interact with the extension.

Also, BTW, I initially thought this feature (common context menu) should be opt-in, but after seeing it and knowing Chrome also enables it by default, I guess we can follow the route.

One thing missing might just be: Add a changelog entry and add yourself to the CONTRIBUTORS.md BTW, @mahmoudhusam. Then we're done!

…xt-menu-feature

mahmoudhusam · 2024-10-23T15:54:41Z

i added my name to to the CONTRIBUTORS.md, but i did not know where to add a changelog entry for my code :

Added

Context Menu Item for QR Code Generation: Implemented a new feature that allows users to generate a QR code for the current page via a context menu item. This feature enhances user experience by providing quick access to QR code generation without navigating through the main UI.
Settings Option to Enable/Disable Context Menu Item: Added a settings option that enables users to toggle the context menu item for QR code generation on or off. This provides users with greater control over their interface and helps customize their experience based on their preferences.

is this good and where should i add it?

rugk · 2024-10-24T18:26:25Z

Thanks yeah, and BTW you don't need to repeat what you've added (and I hope this is no AI summary)

And yeah, that file was not there yet, the Changelog can be added here: https://github.com/rugk/offline-qr-code/blob/master/assets/texts/en/Changelog/next.md (Just like the other changelogs)
Edit: Ah in case that should be the changelog it is somewhat verbose, a shorter one-liner is fine. As said, see how the other changelogs are written.

Feel free to add it there (in a new PR), but I guess I merge it now already.

rugk · 2024-10-31T22:05:31Z

And FYI next time you can (automatically) let issues close when a PR is merged by adding some "magic" text to your PR body. (Manually linked it now.)

Add context-menu item for generating QR code for the current page (ru…

1074bd8

…gk#321)

github-advanced-security bot found potential problems Oct 15, 2024

View reviewed changes

src/background/modules/ContextMenu.js Dismissed Show dismissed Hide dismissed

src/background/modules/ContextMenu.js Dismissed Show dismissed Hide dismissed

rugk reviewed Oct 15, 2024

View reviewed changes

src/_locales/en/messages.json Outdated Show resolved Hide resolved

Update src/_locales/en/messages.json

ba9630f

Co-authored-by: rugk <[email protected]>

rugk requested changes Oct 16, 2024

View reviewed changes

mahmoudhusam added 4 commits October 17, 2024 12:45

Add context-menu item for generating QR code for the current page (ru…

6603f16

…gk#321)

Merge branch 'rugk:master' into context-menu-feature

d637363

Fixed context menu feature, added settings checkbox functionality

48c4a1d

Fixed context menu feature, added settings checkbox functionality DONE

9f52ed1

rugk requested changes Oct 22, 2024

View reviewed changes

src/options/options.html Outdated Show resolved Hide resolved

Fixes based on feedback: Removed custom option saving, added i18n att…

81ba19a

…ributes, improved UX for context menu setting, and moved event listener

rugk approved these changes Oct 23, 2024

View reviewed changes

Add German translation for common context menu

0cff370

mahmoudhusam added 2 commits October 23, 2024 18:44

Added my name to CONTRIBUTORS.md

e8c307d

Merge remote-tracking branch 'origin/context-menu-feature' into conte…

5ebbed7

…xt-menu-feature

rugk approved these changes Oct 24, 2024

View reviewed changes

rugk merged commit 5558b0e into rugk:master Oct 24, 2024
9 checks passed

rugk linked an issue Oct 31, 2024 that may be closed by this pull request

Add context-menu item for generating a QR code for _the current page_ #321

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add context-menu item for generating QR code for the current page (#321) #336

Add context-menu item for generating QR code for the current page (#321) #336

mahmoudhusam commented Oct 14, 2024

rugk left a comment

rugk Oct 15, 2024

mahmoudhusam commented Oct 20, 2024

rugk left a comment

rugk Oct 22, 2024

rugk Oct 22, 2024

rugk Oct 22, 2024

mahmoudhusam commented Oct 23, 2024

rugk left a comment

rugk commented Oct 23, 2024 •

edited

Loading

mahmoudhusam commented Oct 23, 2024

rugk commented Oct 24, 2024 •

edited

Loading

rugk commented Oct 31, 2024

		// Send the current page URL to the popup
		sendQrCodeText(event.pageUrl);

Add context-menu item for generating QR code for the current page (#321) #336

Add context-menu item for generating QR code for the current page (#321) #336

Conversation

mahmoudhusam commented Oct 14, 2024

rugk left a comment

Choose a reason for hiding this comment

rugk Oct 15, 2024

Choose a reason for hiding this comment

mahmoudhusam commented Oct 20, 2024

rugk left a comment

Choose a reason for hiding this comment

rugk Oct 22, 2024

Choose a reason for hiding this comment

rugk Oct 22, 2024

Choose a reason for hiding this comment

rugk Oct 22, 2024

Choose a reason for hiding this comment

mahmoudhusam commented Oct 23, 2024

rugk left a comment

Choose a reason for hiding this comment

rugk commented Oct 23, 2024 • edited Loading

mahmoudhusam commented Oct 23, 2024

Added

rugk commented Oct 24, 2024 • edited Loading

rugk commented Oct 31, 2024

rugk commented Oct 23, 2024 •

edited

Loading

rugk commented Oct 24, 2024 •

edited

Loading