feat(skin): add margin skin (11 themes; light/dark per theme; switchable from Settings)#7721
Draft
JohnMcLear wants to merge 5 commits into
Draft
feat(skin): add margin skin (11 themes; light/dark per theme; switchable from Settings)#7721JohnMcLear wants to merge 5 commits into
JohnMcLear wants to merge 5 commits into
Conversation
…ettings) A standalone drop-in skin alongside colibris / no-skin, with eleven themes — one neutral default (colibris) and five named themes each in light and dark mode: editorial, brutalist, paper, crt, industrial. Theme is chosen via a Theme dropdown injected into the Settings popup (both User Settings and Pad-wide Settings columns) and persists in localStorage under `marginTheme`. The skin applies the saved theme on load — in the pad via pad.js, in the lobby via index.js — so no edits to src/templates/* are required. The skin uses the same CSS-variable contract colibris exposes (`--primary-color`, `--bg-color`, `--main-font-family`, `--editor-horizontal-padding`, …) and reuses the colibris component partials, vendored into `margin/src/` so the skin is fully self-contained. Google Fonts powering the type stacks (Newsreader, Space Mono, Lora, IBM Plex Mono/Sans, VT323, Instrument Serif) are loaded via `@import` from `pad.css` / `index.css` so the skin does not require core template changes.
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
Review Summary by QodoAdd margin skin with 11 switchable themes and Settings integration
WalkthroughsDescription• Adds new "margin" skin with 11 themes (colibris default + 5 named themes in light/dark) • Injects Theme dropdown into Settings popup (User Settings + Pad-wide Settings) • Theme selection persists in localStorage and syncs across pad and lobby • Vendors colibris component partials for full self-contained skin • Supports Google Fonts for themed typography stacks (Newsreader, Space Mono, Lora, IBM Plex, VT323) Diagramflowchart LR
A["User selects theme<br/>in Settings dropdown"] --> B["Theme saved to<br/>localStorage"]
B --> C["data-theme attribute<br/>applied to html"]
C --> D["CSS variables<br/>override per theme"]
D --> E["Pad + lobby<br/>render in theme"]
F["Colibris component<br/>partials vendored"] --> G["margin/src/<br/>self-contained"]
G --> H["No core template<br/>edits needed"]
File Changes1. src/static/skins/margin/pad.js
|
Code Review by Qodo
1. Hardcoded Google Fonts URL
|
The Theme dropdown now lists six themes (Colibris, Editorial, Brutalist, Paper, CRT, Industrial). Light/Dark is exposed as a separate `Dark mode` checkbox in the Settings popup — paired with the theme via a new `data-mode` attribute on <html> so palettes are selected via `[data-theme="X"][data-mode="light|dark"]` in pad.css / index.css. Defaults follow each theme's natural mode (CRT and Industrial start dark; the rest start light) when the user hasn't expressed a mode preference yet. Choices persist in `localStorage` under `marginTheme` and `marginMode`, and propagate to the editor iframes via pad.js. Colibris remains the default and has only a light palette.
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| * Styles the home / pad-list using the same data-theme system as pad.css. | ||
| * Drop alongside pad.css in src/static/skins/margin/. */ | ||
|
|
||
| @import url("https://fonts.googleapis.com/css2?family=Newsreader:ital,opsz,wght@0,6..72,300..700;1,6..72,300..700&family=Instrument+Serif:ital@0;1&family=Lora:ital,wght@0,400..700;1,400..700&family=Space+Mono:ital,wght@0,400;0,700;1,400;1,700&family=VT323&family=IBM+Plex+Mono:ital,wght@0,400;0,500;0,600;1,400&family=IBM+Plex+Sans:ital,wght@0,400;0,500;0,600;0,700&display=swap"); |
There was a problem hiding this comment.
1. Hardcoded google fonts url 📘 Rule violation ⛨ Security
index.css and pad.css hardcode https:// Google Fonts @import URLs instead of using protocol-independent // URLs. This violates the guideline intended to avoid protocol-specific URL embedding so the resource works under both HTTP and HTTPS.
Agent Prompt
## Issue description
Update the Google Fonts `@import` statements in `index.css` and `pad.css` to avoid hardcoding the `https://` protocol and instead use protocol-independent `//` URLs as required by the compliance guideline.
## Issue Context
PR Compliance ID 7 requires protocol-independent URLs (`//`) where applicable; these are fetchable external stylesheet URLs (`fonts.googleapis.com`) that can be expressed in protocol-independent form so they work under both HTTP and HTTPS.
## Fix Focus Areas
- src/static/skins/margin/index.css[5-5]
- src/static/skins/margin/pad.css[25-25]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Member
Author
There was a problem hiding this comment.
Addressed in 6a7242b — Google Fonts @import URLs in pad.css and index.css are now protocol-relative (//fonts.googleapis.com/…).
Addresses Qodo feedback on PR #7721: use //fonts.googleapis.com/… rather than hardcoded https:// so the resource works under both HTTP and HTTPS without protocol-specific embedding (PR Compliance ID 7).
…er theme) - Build recent-pad links with new URL() + encodeURIComponent so a trailing slash, query, hash, or special chars in pad names no longer produce a broken link. - Wrap recentPads read/parse and writes in try/catch on both the lobby and pad scripts; an exception used to abort customStart() and break recent pads / settings injection in private mode or when the entry was corrupted. - Bootstrap data-theme + data-mode on the timeslider page (and inherit from the parent doc when running inside #history-frame), since margin/pad.css scopes its theme tokens under [data-theme="..."]. History was unthemed.
Member
Author
|
Addressed the three Qodo bugs in 08a25c9:
The Google Fonts rule violation (#1) was already fixed in 6a7242b. |
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Potential PR
This PR rose from a conversation between me and Sam and an interest in Claude Designer. The PR adds additional Themes and a selector in Settings. Each Theme has been tested w/ plugins and has dark/light mode.
To Try the themes click Settings > Themes > Select the Theme you want then for Dark mode click the Dark mode toggle.
Summary
Adds a new third-party-friendly skin alongside
colibris/no-skin, with eleven themes — one neutral baseline (colibris) and five named themes each in light and dark mode:colibriseditorialeditorial-darkbrutalistbrutalist-darkpaperpaper-darkcrt-lightcrtindustrial-lightindustrialThe skin uses the same CSS-variable contract
colibrisexposes (--primary-color,--bg-color,--main-font-family,--editor-horizontal-padding, …) and re-uses the colibris component partials, vendored intomargin/src/so the skin is fully self-contained.A Theme dropdown is injected by
margin/pad.jsinto both columns of the Settings popup (User Settings + Pad-wide Settings) and matches the markup/styling of the built-in Font type / Language rows. Selecting a theme persists the choice inlocalStorageunder themarginThemekey and reflects across the pad and the lobby.Why this is drop-in
colibris) on load — in the pad viapad.js, in the lobby viaindex.js. Google Fonts powering the type stacks (Newsreader / Space Mono / Lora / IBM Plex Mono/Sans / VT323 / Instrument Serif) are loaded via@importfrompad.css/index.css.src/static/skins/margin/."skinName": "margin"insettings.json, same mechanism as the other skins.Test plan
pnpm run plugins i ep_comments_page ep_webrtc ep_font_family ep_font_size ep_font_color ep_headings2 ep_set_title_on_pad ep_align ep_image_upload ep_subscript_and_superscript+ boot etherpad withskinName=margin; lobby + pad render in the saved/default theme.--editor-horizontal-padding/--editor-vertical-paddingzero out so the editor fills the container, matching colibris's responsive behavior.🤖 Generated with Claude Code