fix: adjust css file loading #7
Conversation
|
📦 Extension packages built successfully! |
rise-erpelding
changed the title
dustin-jw
left a comment
dustin-jw
left a comment
There was a problem hiding this comment.
Everything works in Firefox and Chrome, but there are a couple areas that could be cleaned up a bit. I like the approach with grouping together all the common CSS things in JS though, nice work!
src/core/ExtensionManager.js
Outdated
| // Add a marker to indicate core CSS bundle is loaded | ||
| const marker = document.createElement('style'); | ||
| marker.id = 'carbon-visualizer-core-css-bundle'; | ||
| marker.textContent = '/* Core CSS bundle loaded */'; |
There was a problem hiding this comment.
Could we refactor this so we just have the one style tag with #carbon-visualizer-core-css-bundle that contains all the styles from the other files? As-is, we get 6 other unidentified style tags that are harder to find when debugging.
let css = '';
for (const file of files) {
const cssUrl = this.browserAPI.runtime.getURL(file);
const response = await fetch(cssUrl);
css += await response.text();
}
const style = document.createElement('style');
style.textContent = css;
style.id = 'carbon-visualizer-core-css-bundle';
document.head.appendChild(style);There was a problem hiding this comment.
What do we think about a style tag for the bundle, then adding one for the css for each panel?
I'm not sure performance-wise it makes a huge difference whether you throw all the panel styles in the bundle or keep them separate. It just felt slightly more logical to have panel styles separate, but it seems reasonable to bundle them too, so I'm on the fence and happy to go either way.
There was a problem hiding this comment.
I'd be happy to lump them all together. Keeping them separated is more useful on the authoring side, but bundling them on the consuming side should be fine, and it would simplify Panel.js a bit.
src/core/Panel.js
Outdated
| results: { | ||
| htmlFile: 'src/panels/results/results.html', | ||
| cssFile: 'src/panels/results/results.css', | ||
| cssFile: 'src/styles/results.css', |
There was a problem hiding this comment.
Technically out of scope, but I found that every time I changed the panel, like toggling between welcome and results, it would add another redundant style tag with the same ID.
We could add a check at the start of loadPanelCSS() to avoid injecting CSS that has already been loaded, the same way loadCoreCSS does.
dustin-jw
left a comment
dustin-jw
left a comment
There was a problem hiding this comment.
Looks good, thanks @rise-erpelding!
Summary
Refactors CSS loading to use a modular design system approach by loading individual design token and theme files in
ExtensionManager.jsinstead of a monolithic styles file, then removes the now-unusedstyles.cssfile for cleaner project structure.BLDL-25
Problem
The extension was loading styles from a single
styles.cssfile that contained@importstatements with Chrome-specificchrome-extension://URLs. This caused styles to fail loading in Firefox, which uses themoz-extension://protocol instead.Solution
Split the monolithic approach into a modular design system:
ExtensionManager.loadCoreCSS()to load design tokens and themes individuallysrc/styles/styles.css- The old file with@importstatements is no longer neededAll files are loaded via JavaScript using
browserAPI.runtime.getURL(), which handles browser-specific protocols automatically, ensuring cross-browser compatibility.Testing
<style>tags in the page's<head>withcarbon-visualizer-prefixed ids are visible.