Skip to content

fix: adjust css file loading #7

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.

Sign up for GitHub

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

Merged
dustin-jw merged 6 commits into from
Dec 2, 2025
Merged

fix: adjust css file loading #7

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c496778
fix: adjust css file paths
rise-erpelding Nov 7, 2025
5c01feb
fix: load all the things
rise-erpelding Nov 11, 2025
9584dc8
chore: delete styles.css
rise-erpelding Nov 11, 2025
83b6143
fix: load styles one time only
rise-erpelding Nov 18, 2025
1ac1059
chore: bundle styles during build
rise-erpelding Nov 19, 2025
c1d72da
chore: remove panel css from configs, load in bundle
rise-erpelding Nov 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 25 additions & 9 deletions src/core/ExtensionManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,17 +53,33 @@ class ExtensionManager {
}

async loadCoreCSS() {
if (document.getElementById('carbon-visualizer-core-css')) return;
if (document.getElementById('carbon-visualizer-core-css-bundle')) return;

try {
const cssUrl = this.browserAPI.runtime.getURL('src/styles/core.css');
const response = await fetch(cssUrl);
const css = await response.text();

const style = document.createElement('style');
style.id = 'carbon-visualizer-core-css';
style.textContent = css;
document.head.appendChild(style);
const files = [
'src/styles/tokens/color.css',
'src/styles/tokens/size.css',
'src/styles/tokens/typography.css',
'src/styles/themes/default.css',
'src/styles/generic/typography.css',
'src/styles/core.css'
];

for (const file of files) {
const cssUrl = this.browserAPI.runtime.getURL(file);
const response = await fetch(cssUrl);
const css = await response.text();

const style = document.createElement('style');
style.textContent = css;
document.head.appendChild(style);
}

// 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 */';
Copy link
Contributor

@dustin-jw dustin-jw Nov 18, 2025

Choose a reason for hiding this comment

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

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);

Copy link
Contributor Author

@rise-erpelding rise-erpelding Nov 19, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@dustin-jw dustin-jw Nov 19, 2025

Choose a reason for hiding this comment

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

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.

document.head.appendChild(marker);
} catch (error) {
console.error(error);
}
Expand Down
4 changes: 2 additions & 2 deletions src/core/Panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,14 @@ class Panel {
const configs = {
welcome: {
htmlFile: 'src/panels/welcome/welcome.html',
cssFile: 'src/styles/styles.css',
cssFile: 'src/styles/welcome.css',
jsFile: 'src/panels/welcome/welcome.js',
containerId: 'carbon-visualizer-welcome-panel',
className: 'cv-panel--welcome'
},
results: {
htmlFile: 'src/panels/results/results.html',
cssFile: 'src/panels/results/results.css',
cssFile: 'src/styles/results.css',
Copy link
Contributor

@dustin-jw dustin-jw Nov 18, 2025

Choose a reason for hiding this comment

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

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.

Image

We could add a check at the start of loadPanelCSS() to avoid injecting CSS that has already been loaded, the same way loadCoreCSS does.

jsFile: 'src/panels/results/results.js',
containerId: 'carbon-visualizer-results-panel',
className: 'cv-panel--results'
Expand Down
16 changes: 0 additions & 16 deletions src/styles/styles.css
View file
Open in desktop

This file was deleted.