feat(theme-handler): browser://theme/ Protocol for Cross-Browser Theming with Base16 #291
Conversation
|
Couldn't test it locally because of this #287 |
app/protocols/index.js
Outdated
| @@ -90,6 +93,10 @@ export async function setupProtocols (session) { | |||
| sessionProtocol.registerStreamProtocol('agregore', browserProtocolHandler) | |||
There was a problem hiding this comment.
Rename the current browserProtocolHandler to the agregoreProtocolHandler, I think this will make the code more readable and more obvious.
app/protocols/index.js
Outdated
| @@ -90,6 +93,10 @@ export async function setupProtocols (session) { | |||
| sessionProtocol.registerStreamProtocol('agregore', browserProtocolHandler) | |||
| globalProtocol.registerStreamProtocol('agregore', browserProtocolHandler) | |||
|
|
|||
| const { handler: themeProtocolHandler } = await createThemeHandler() | |||
There was a problem hiding this comment.
Rename the themeHandler to be the Agregore theme handler.
| const parsedUrl = new URL(url) | ||
| let filePath | ||
|
|
||
| if (parsedUrl.hostname === 'theme') { |
There was a problem hiding this comment.
We should have a check for pathname === '/vars.css' and generate it dynamically the way we do in the current handler here: https://github.com/AgregoreWeb/agregore-browser/blob/master/app/protocols/browser-protocol.js#L76
That's also where the base16 vars should get defined.
app/protocols/theme-handler.js
Outdated
| filePath = path.join(__dirname, '../pages/theme', fileName) | ||
| console.log('Attempting to serve:', filePath) | ||
|
|
||
| if (!fs.existsSync(filePath)) { |
There was a problem hiding this comment.
Avoid using sync methods whenever possible. In this case we should use the existing resolveFile logic from the agregore handler. https://github.com/AgregoreWeb/agregore-browser/blob/master/app/protocols/browser-protocol.js#L154
| data | ||
| }) | ||
| } else { | ||
| sendResponse({ |
There was a problem hiding this comment.
This should show the 404 page like in the agregore handler" https://github.com/AgregoreWeb/agregore-browser/blob/master/app/protocols/browser-protocol.js#L136
app/protocols/theme-handler.js
Outdated
| @@ -0,0 +1,54 @@ | |||
| import path from 'path' | |||
| import { fileURLToPath } from 'url' | |||
| import fs from 'fs' | |||
There was a problem hiding this comment.
Whenever you read from the file system, make sure you use ScopedFS otherwise users can use malicious path names like ../../../config/yourpasswords to extract arbitrary files from the filesystem
| @@ -69,3 +69,91 @@ code { | |||
| ``` | |||
|
|
|||
| This font provides built-in syntax highlighting for code blocks, making it easier to read and understand code snippets. | |||
|
|
|||
| ## Theme Protocol (`browser://theme/`) | |||
There was a problem hiding this comment.
This is great! Gotta remember to update the website docs with this too.
https://github.com/AgregoreWeb/website/blob/main/docs/theming.md
There was a problem hiding this comment.
Oh, I added base16 to Peersky (p2plabsxyz/peersky-browser#43). I meant to say here (with Agregore standards) - once I add the dynamic vars.css generation (f1c3852).
|
|
I actually ended up doing something more simple: I registered the existing agregore handler as |
Related Issue: p2plabsxyz/peersky-browser#37
Related PR: p2plabsxyz/peersky-browser#43
@RangerMauve can you add base16 here? #289