-
Notifications
You must be signed in to change notification settings - Fork 0
feat: Browser Extension Side Panel #2
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
Conversation
a7f5cb6
to
62bc8e5
Compare
f9ea420
to
e70cdd3
Compare
], | ||
"host_permissions": [ | ||
"https://www.googleapis.com/*", | ||
"https://api.thegreenwebfoundation.org/*" |
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.
Assuming this will be replaced, fine for 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.
The colors could be defined at the root level in CSS custom properties, which would save some repetition and make it easier to find/change the definitions. Also the color contrast is not accessible.
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.
After talking with @marissahuysentruyt, I am capturing the styling comments in this PR to document in Jira, as they don't need to be addressed as part of this PR, but we want to make sure they are addressed.
There is a card on the board from the EcoGraders team to address the CSS structure for the extension, so that will be addressed there.
I was out last week when the decision was made to use an AI boilerplate, but I'm assuming that the colors that exist here were auto generated and aren't the final colors we will be using in the app.
src/panels/welcome/welcome.css
Outdated
} | ||
|
||
.carbon-visualizer-btn--primary:active { | ||
transform: translateY(0); |
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.
What's with the transform? 1px doesn't seem worth it?
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.
Leaving as is for now- we'll fix this in a future PR.
src/panels/welcome/welcome.css
Outdated
padding: 0.75rem; | ||
background: rgba(255, 255, 255, 0.1); | ||
border-radius: 0.5rem; | ||
border: 1px solid rgba(255, 255, 255, 0.2); |
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.
Lot of opacity stuff going on that I wouldn't recommend, why not use colors that work at full opacity.
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'm going to remove most of the opacities. 👍
background.js
Outdated
|
||
} catch (injectError) { | ||
// Silently handle injection errors | ||
} |
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.
What are the errors that we're silently catching? Is this necessary?
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.
Seems fine at a glance, would be nice to have this run in CI to package things up and put them in a container registry or something.
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.
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.
Leaving comments, and will await our decisions on the Firefox patch, the nested try/catch, and the potential duplicate messageListener.
@media (max-width: 480px) { | ||
.carbon-visualizer-panel { | ||
width: 100%; | ||
} | ||
} |
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.
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.
More styles and nit-picky things will be addressed in future PRs.
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.
Yes we shouldn't need to worry about any styling in this PR. All styling will be changed according to the prototype in future CSS stories.
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 noticed in the README that we will eventually have more panels for each view. I've kept the welcome styles separate from the core/shared styles, and pulled some of the welcome-specific selectors back into the welcome CSS. 😮💨
Let me know what you think.
src/panels/welcome/welcome.css
Outdated
padding: 0.75rem; | ||
background: rgba(255, 255, 255, 0.1); | ||
border-radius: 0.5rem; | ||
border: 1px solid rgba(255, 255, 255, 0.2); |
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'm going to remove most of the opacities. 👍
src/core/ExtensionManager.js
Outdated
this.lastToggleTime = now; | ||
|
||
try { | ||
const existingPanelInDOM = document.querySelector('.carbon-visualizer-panel--welcome'); |
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.
Instead of querying the DOM, could we use this.panels.get()
? It looks like we use both approaches and I was wondering if there's a reason to use both or if we should use the same approach in all of the functions.
DOM querying happens in togglePanel
, cleanupOrphanedPanels
and closeAllPanels
.
I see this.panels.get()
(is that a panels map, then?) is used in openPanel
, closePanel
README.md
Outdated
- **Modern Architecture**: Modular ES6 class-based design with clean separation of concerns | ||
- **Cross-Browser Support**: Works seamlessly on Chrome and Firefox (Manifest V3) | ||
- **Smooth Animations**: Professional slide-in/out panel with CSS transitions | ||
- **Robust Toggle System**: Debounced clicks with concurrency protection |
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.
since we thought we probably didn't need the debounce code, I can safely remove this debounce feature description?
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.
Removed.
README.md
Outdated
- **Modular Panels**: Each panel (welcome, loading, results) has its own HTML/CSS/JS files | ||
|
||
### **Robust Toggle System** | ||
- **Debouncing**: Prevents rapid clicks (500ms ExtensionManager, 800ms background) |
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.
Did i miss it- where was the debounce stuff in background.js?
noting that we can possible remove this if we don't think it's necessary
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.
Removed.
@@ -0,0 +1,49 @@ | |||
// Cross-browser background script support | |||
// Note: Firefox MV3 uses background.scripts instead of service_worker |
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 had Cursor create a patch function for this so I stopped running into this background.scripts
error in Firefox. I'll put a comment on the build.js file.
fs.copyFileSync('manifest.json', path.join(chromeDir, 'manifest.json')); | ||
fs.copyFileSync('manifest.json', path.join(firefoxDir, 'manifest.json')); | ||
} | ||
|
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.
So this should keep the single manifest file, and then just update background.service_worker
with background.scripts
accordingly, in the manifest for Firefox.
// Patch Firefox manifest for browser-specific settings
function patchFirefoxManifest() {
const firefoxManifestPath = path.join(firefoxDir, 'manifest.json');
const manifest = JSON.parse(fs.readFileSync(firefoxManifestPath, 'utf-8'));
// Change service_worker to scripts for background
manifest.background.scripts = [manifest.background.service_worker];
delete manifest.background.service_worker;
// Add browser-specific settings for Firefox
manifest.browser_specific_settings = {
gecko: {
"id": "[email protected]"
}
};
fs.writeFileSync(firefoxManifestPath, JSON.stringify(manifest, null, 2));
}
If we like this approach, I can push up a commit with this. The browser-specific bit was in the original manifest-firefox.json file too.
createDirectories(); | ||
copyCommonFiles(); | ||
copyManifests(); | ||
|
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.
(this is where we'd also run the firefox patch function)
src/styles/core.css
Outdated
/* High contrast mode support */ | ||
@media (prefers-contrast: high) { | ||
.carbon-visualizer-panel { | ||
border: 2px solid #000000; |
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.
This is what I'm seeing with prefers-contrast: more
turned on:
Screen.Recording.2025-10-08.at.9.12.20.AM.mov
However, do we want to address any forced-colors: active
styling? As in, at least give it a background color?
Screen.Recording.2025-10-08.at.12.26.53.PM.mov
this.lastToggleTime = now; | ||
|
||
try { | ||
const existingPanelInDOM = document.querySelector('.cv-panel--welcome'); |
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.
Instead of querying the DOM, could we use this.panels.get()
? It looks like we use both approaches and I was wondering if there's a reason to use both or if we should use the same approach in all of the functions.
DOM querying happens in togglePanel
, cleanupOrphanedPanels
and closeAllPanels
.
I see this.panels.get()
(is that a panels map, then?) is used in openPanel
, closePanel
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.
Yes, this is a panels map. We should be able to use this in place of the DOM query. Given the time restrictions we have prior to Build Lab this afternoon, I think it's ok to make that refactor as part of a separate PR.
/* High contrast mode support */ | ||
@media (prefers-contrast: more) { | ||
.cv-panel { | ||
border: 2px solid var(--cv-black); | ||
} | ||
} |
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.
#2 (comment) (my comment was marked "outdated" but it's not really.)
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.
TODO: don't worry about the styles so much, but make sure the style comments are captured somewhere in the board.
just make sure this works in the browsers! Styling/wording/features are probably handled elsewhere! |
/* Core styles for Carbon Visualizer extension */ | ||
:root { | ||
/* greens */ | ||
--cv-primary-green: hsl(114deg 64.9% 32.0%); |
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.
leave a code comment about changing any of these color properties
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.
Done.
Dismissing my own review since I'm committing instead 👍
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.
Thanks for these changes @marissahuysentruyt! I am going to capture the styling notes in Jira.
See my comment on this.panels - I think we should change ExtensionManager use this instead of the DOM queries, but I know we are short on time for build lab today. I think this is ok to merge for the moment, and then you can open another PR to make that change when you have time. I would normally not do it that way, but given the time restrictions for Build Lab, I think this is ok to do since these changes should not be a blocker to having people move forward.
- use h1 for title instead of h2 in header section - use h2 for content section instead of h3 - update list of features to use ul and li tags instead of divs
- remove debounce functionality - use 'cv' in place of 'carbon-visualizer' in class names - remove debounce from background.js (it was deemed unnecessary) - remove debounce language from README - log errors in catch blocks (no longer silently handle errors) - no longer silently handle errors, and console.errors instead - adds exit transition to panel - remove redundant messageListener - remove nested try/catch in background.js - patch background.scripts in firefox manifest
- remove shared styles in welcome.css and adds them to core.css - creates green color palette with custom properties (with a CSS code comment that those colors are temporary) - renames classes from carbon-visualizer to cv
5449c40
to
f968c6e
Compare
This PR is an AI-generated boilerplate starter for the Carbon Visualizer browser extension side panel. It should support both Chrome and Firefox and implement core functionality to toggle a side panel open and closed when the extension icon is clicked.