Skip to content

Added Configuration Option #2 and Upgraded Extension to Follow Chrome Extension Best Practices#3

Open
9jaDevo wants to merge 2 commits into
ao5357:masterfrom
9jaDevo:master
Open

Added Configuration Option #2 and Upgraded Extension to Follow Chrome Extension Best Practices#3
9jaDevo wants to merge 2 commits into
ao5357:masterfrom
9jaDevo:master

Conversation

@9jaDevo

@9jaDevo 9jaDevo commented Nov 15, 2024

Copy link
Copy Markdown

This pull request introduces the following changes to the extension:

1. Configuration Option #2

  • A new option in the settings page that allows users to save and apply their preferred font style. The selected font (either "Redacted Script" or "Redacted Standard") is now persisted across sessions using chrome.storage.sync.
  • Users can save the font choice through the settings page, and the selected font is immediately applied upon saving.

2. Upgrade Extension to Follow Chrome Extension Best Practices:

  • Refined the background script and content script injection process. The content script is now injected only when necessary (i.e., when the user clicks the extension icon), ensuring a better user experience.
  • Implemented better error handling for message passing between background and content scripts. This prevents issues when the content script is not loaded or when message passing fails.
  • Ensured that styles are applied only to the current tab, and added an option to disable styles completely. The disable feature now works reliably, resetting the page styles even after a page reload.
  • Incorporated best practices for Chrome extension manifest version 3, ensuring that the extension complies with the latest standards and API usage.

New Features:

  • Added Disable style button. When clicked, it correctly disables the applied styles, even after the page reloads.

Additional Notes:

  • This update improves the stability and user control over the extension. Users now have a more seamless experience in customizing and managing font styles on web pages.
  • We've followed Chrome’s best practices for managing extension resources and improving performance, ensuring a more optimized experience overall.

I added the Configuration Option and Upgrade Extension to follow best practices for Chrome extensions.
@ao5357

ao5357 commented Nov 15, 2024

Copy link
Copy Markdown
Owner

@9jaDevo you rock! I will review this either today or early tomorrow. From a quick look it's fantastic and I definitely appreciate the detailed PR description.

@9jaDevo

9jaDevo commented Nov 21, 2024

Copy link
Copy Markdown
Author

@ao5357 I trust you are good. I guess it has been a busy period. I just want to confirm if you were able to review the code.

@ao5357

ao5357 commented Nov 22, 2024

Copy link
Copy Markdown
Owner

@ao5357 I trust you are good. I guess it has been a busy period. I just want to confirm if you were able to review the code.

Yes, sorry, reviewing this PR is important and among the next 3 things I have planned to do outside of my day job. The past two weeks have really gotten away from me and I feel bad for letting this sit here since you so quickly did an incredible thing. It's looking like a tonight or tomorrow morning thing now -- thank you for your patience

@ao5357 ao5357 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

@9jaDevo sorry for the delay in getting back to you.

I definitely appreciate all the work here, and hope we can get this to a mergeable state.

The key thing that appears to be missing for me is that the extension doesn't seem to work.

Testing steps:

  1. Reviewed code line-by-line. Made sure that there wasn't any obvious third-party injection since that's my main concern with open source contributions on these extensions. Everything looked great
  2. Cloned down git@github.com:9jaDevo/page-to-wireframe.git to my local
  3. In Chrome went to chrome://extensions and loaded the checked out directory as an unpacked extension
  4. Navigated to a few pages and clicked on the extension icon. Was presented with a settings form, but clicking either 'Save' or 'Disable' did nothing to the pages I was on

When I first opened the browser devtools it gave a lot of warnings about the tabId not being found, but it didn't seem to be triggered by the extension action -- just perhaps by having tabs.

The intent of Issue #2 was giving the user the ability to choose their preferred of the two fonts once when setting up the extension (or right-clicking the extension icon) in a more persistent manner -- it's not really an option you'd have to pick every time you click the extension icon. This can be accomplished with the options_page and/or options_ui manifest options that are v2/v3 compatible. Here's an example https://github.com/ao5357/development_favicon/blob/master/manifest.json

When the user clicks the extension icon it should only turn the page into a wireframe. Reloading the page should not persist the wireframe -- the user can re-click the extension icon to make it happen again as desired. It's a very simple, predictable operation. I wouldn't be opposed to the wireframe going away if the user clicks the icon a second/even time in a single page load, but it's not a huge priority.

I'm cool with most of the formatting changes you're suggesting in the PR, but would prefer to keep Helvetica in the CSS font stacks. If we're not bringing in an extension-specific font then I'd like a full web-safe Arial/Helvetica stack.

@9jaDevo

9jaDevo commented Nov 25, 2024

Copy link
Copy Markdown
Author

@ao5357 Thanks for your Feedback; kindly check the video below of how it is working here; maybe I might need to commit the code again
https://www.loom.com/share/fc5579b37e824527852dda41410c1b60

@9jaDevo

9jaDevo commented Nov 29, 2024

Copy link
Copy Markdown
Author

Here's my response to the points you raised:

1. Extension Not Working

  • I've made the following adjustments:
    • Font Selection: The font switching logic will now persist the selected font using chrome.storage.sync. This ensures that the user's preference for a font is retained across page loads and doesn't require selecting a font every time the extension icon is clicked.
    • Wireframe Toggling: The wireframe toggle should now work as expected. When the extension icon is clicked, it will turn the page into a wireframe and revert to normal upon another click.

2. Persistent Font Selection (Issue #2)

  • The original issue you referenced (Issue Add configuration option to use Redacted regular instead of Redacted script #2) was regarding allowing the user to select their preferred font once and have it persist without needing to re-select it each time.
  • I've added functionality to manage font selection via chrome.storage.sync, which ensures that once the user selects their font preference, it is saved persistently and doesn't require re-selection on every click of the extension.
  • This can be accomplished by using the options_page or options_ui in the manifest to provide the user with a settings page for persistent font selection.

3. Extension Behavior on Reload

  • You mentioned that the wireframe styles should not persist when the page reloads. I've updated the logic to ensure that clicking the extension icon toggles the wireframe view on and off without persisting it across page reloads. The user will need to click the extension icon again to activate the wireframe after a reload.
  • I've also added the option for the wireframe to be toggled off on a second click, though this isn't a top priority and can be revisited later if necessary.

4. Font Stack

  • I understand the preference for keeping the Helvetica font in the CSS font stack. I've maintained Helvetica, Arial, sans-serif in the global font family as a fallback, ensuring a web-safe and predictable font stack.
  • The custom fonts (Redacted Script and Redacted Standard) will only be applied when the extension is toggled, and these will override the default font stack.

Additional Changes:

  • CSS Injection: The extension now injects the font.css file on page load to handle font-related styles and styles.css dynamically when the extension is toggled, applying the wireframe styles.

@9jaDevo
9jaDevo requested a review from ao5357 November 29, 2024 02:27
@ao5357

ao5357 commented Dec 2, 2024

Copy link
Copy Markdown
Owner

@9jaDevo Thanks! I should have time to review the changes this week.

@ao5357

ao5357 commented Dec 9, 2024

Copy link
Copy Markdown
Owner

@9jaDevo I just reloaded the updated extension, both unpacked and locally packed, and it's still not cooperating.

Screenshot from 2024-12-08 21-48-04
Screenshot from 2024-12-08 21-47-01

Should the extension work immediately upon installation? Could you provide any additional steps required to get it working? If it helps I'm using Chrome 131 on Linux.

The current version uses CSS and other URL formats like url('chrome-extension://__MSG_@@extension_id__/css/fonts/redacted/redacted-script-light.woff2') format('woff2'); -- could the chrome-extension://__MSG_@@extension_id__/ explicit prefix help to ensure the resources are found?

Also, it seems the extension inserts at least one stylesheet (and maybe more stuff like content scripts?) on every pageload of every tab. Is this necessary? Ideally the extension would only do an action when a user clicks the button -- this would help with performance, privacy, security, etc.

@9jaDevo

9jaDevo commented Dec 10, 2024

Copy link
Copy Markdown
Author

@ao5357 Actually, before I deployed, I ensured it went through testing. I am also running Chrome version 131 on Windows 11, and all is running smoothly; I will try to find a way to run a test on Linux.
Below is a video of the test I ran on a new Chrome profile
https://youtu.be/PlztmQ9gbVY

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants