feat: initial design system - #1
thomasjammet wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Why are we building for the player component?
The player should build its own based on web-ui.
There was a problem hiding this comment.
Yes I needed to factorize players styles somewhere, because we have wrts-client/webrtc-client/videojs-plugins and more.
It's not bundled in styles.css though.
I see no better place for now
There was a problem hiding this comment.
The fonts certainly bring their own license, did you check that they are the same? If not, please include the licence from the fonts into the fonts folder or include them here.
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| workflow_run: | ||
| workflows: [lint] | ||
| types: [completed] | ||
| branches: [main] |
There was a problem hiding this comment.
🔴 release.yml's workflow_run trigger listens for workflows: [lint] (lowercase), but lint.yml declares name: Lint (capital L). GitHub Actions matches this field case-sensitively against the upstream workflow's name, so the release job will never fire automatically after Lint completes on main — only the manual workflow_dispatch path works, defeating the PR's stated auto-release-after-lint flow.
Extended reasoning...
The bug: release.yml (lines 3-6) declares:
on:
workflow_run:
workflows: [lint]
types: [completed]
branches: [main]while lint.yml (line 1) declares name: Lint (capital L). GitHub Actions resolves the workflow_run.workflows list against the referenced workflow's name: field using an exact, case-sensitive string comparison — there is no fallback to the file path once a name is set, and no case-folding. 'lint' !== 'Lint', so this trigger will never match, and the workflow_run event will simply never be delivered for this workflow.
Why nothing else catches it: The job's if: condition (line 19) is:
if: ${{ (github.event_name == 'workflow_run' && github.event.workflow_run.conclusion == 'success') || (github.event_name == 'workflow_dispatch' && github.ref == 'refs/heads/main') }}This is a correctness check on the payload of an event that GitHub Actions will never actually send here, because the trigger match happens further upstream (at the 'does this workflow_run event apply to this workflow file' resolution step, before any job/if evaluation runs). There's no lint, test, or schema validation for workflow YAML that would catch a workflow-name mismatch like this — it fails completely silently, with no error surfaced anywhere in the Actions UI.
Impact: The PR's description states the intended flow explicitly: release.yml runs semantic-release 'once lint passes on main'. With this mismatch, that automatic path is dead on arrival. The only way a release can be cut is by manually triggering workflow_dispatch from the main branch. Since publishing @ceeblue/web-ui@1.0.0 is the PR's primary purpose, a maintainer merging as-is would see every future merge to main complete its Lint run and then... nothing — no release, no error, no obvious signal that anything is wrong, until someone notices packages aren't being published.
Step-by-step proof:
- A commit is pushed to
main. - GitHub runs the workflow defined in
.github/workflows/lint.yml, whosename:field isLint. - Lint completes successfully, which would normally emit a
workflow_runevent withworkflow_run.completed" targeting any workflow file that lists a matching name in itsworkflow_run.workflows` array. - GitHub checks
release.yml'sworkflows: [lint]array against the completed workflow's name,'Lint'. Since the comparison is case-sensitive and'lint' != 'Lint',release.ymlis not considered a subscriber to this event. release.ymlis never invoked; theif:guard on the release job (which correctly checks forworkflow_runsuccess) is never even reached.- The only way to run
release.ymlgoing forward is to manually dispatch it viaworkflow_dispatch, which requires a human to remember to do so after every merge — the opposite of the described automation.
The fix is a one-line change: either update release.yml to workflows: [Lint] (matching the exact case of lint.yml's name: field), or lowercase name: Lint to name: lint in lint.yml. Given lint.yml's job id and file name are already lowercase, aligning release.yml to reference Lint is the more consistent fix, but either works.
| // Self-hosted fonts: the stylesheet + its woff2 files | ||
| cpSync('src/fonts', 'dist/fonts', { recursive: true }); | ||
| writeFileSync('dist/fonts.css', banner + readFileSync('src/styles/fonts.css', 'utf8').trimEnd() + '\n'); |
There was a problem hiding this comment.
🔴 Self-hosted Poppins/JetBrains Mono fonts (OFL-licensed per fonts.css's own comment) are copied into dist/fonts/ by build.mjs:35 without their required OFL license text — src/fonts/ has no license file, and the repo's top-level LICENSE (AGPL-3.0-or-later) doesn't cover third-party fonts. Reviewer jobl already flagged this exact gap on the LICENSE file in the PR discussion; add an OFL.txt (with the appropriate copyright/reserved-font-name notices) to src/fonts/ so it gets picked up by the existing cpSync call.
Extended reasoning...
The bug: build.mjs builds the publishable dist/ output that becomes the @ceeblue/web-ui npm package (package.json files: [\"dist\"]). Lines 34-36 handle the self-hosted fonts:
// Self-hosted fonts: the stylesheet + its woff2 files
cpSync('src/fonts', 'dist/fonts', { recursive: true });
writeFileSync('dist/fonts.css', banner + readFileSync('src/styles/fonts.css', 'utf8').trimEnd() + '\n');This copies whatever is in src/fonts/ verbatim into dist/fonts/. Today src/fonts/ contains exactly six files: poppins-400/500/600/700.woff2 and jetbrains-mono-400/500.woff2 — no license file of any kind.
Why this matters: src/styles/fonts.css itself documents the provenance in its header comment: "Both fonts are OFL-licensed." The SIL Open Font License requires that its license text (and the font's copyright/Reserved Font Name notice) accompany every copy of the Font Software that is redistributed, whether alone or bundled with other software. The repository's top-level LICENSE file is AGPL-3.0-or-later (see the banner build.mjs stamps on every output: /*! @ceeblue/web-ui vX.Y.Z | AGPL-3.0-or-later */), which governs Ceeblue's own code but does not — and cannot — relicense third-party font binaries under a different license. So the AGPL LICENSE file at the repo root does not satisfy OFL's accompanying-license requirement for the fonts.
Proof / walkthrough:
- Run
npm run build(or the semantic-releaseprepareCmd, which is what actually fires on merge tomain). build.mjs:35executescpSync('src/fonts', 'dist/fonts', { recursive: true }).- Inspect
dist/fonts/: it contains the six.woff2binaries and nothing else — noOFL.txt, no copyright notice. package.json'sfilesfield is[\"dist\"], and the PR description states merging this PR publishes@ceeblue/web-ui@1.0.0to the public npm registry via the trusted-publishing release workflow.- Result: the published npm tarball redistributes OFL-licensed font binaries with no accompanying OFL license text, which is a violation of the license terms those fonts were obtained under.
Why nothing currently prevents this: there's no lint rule, build check, or file that would fail if src/fonts/ is missing a license — cpSync copies whatever exists, silently. Reviewer jobl raised exactly this concern in the PR timeline (inline comment on LICENSE: "The fonts certainly bring their own license, did you check that they are the same? If not, please include the licence from the fonts into the fonts folder or include them here."), and it remains unaddressed — no license file has been added to src/fonts/ and no comment in the diff explains it was intentionally omitted.
Fix: add an OFL.txt (the standard SIL Open Font License 1.1 text, with the Poppins and JetBrains Mono copyright/Reserved Font Name lines from their respective upstream releases) into src/fonts/. No code change to build.mjs is needed — the existing cpSync('src/fonts', 'dist/fonts', { recursive: true }) call will pick it up automatically and ship it alongside the binaries.
| // player.css: granular copy in css/ + a stamped standalone at dist/player.css. | ||
| const player = readFileSync('src/styles/player.css', 'utf8'); | ||
| writeFileSync('dist/css/player.css', player); | ||
| writeFileSync('dist/player.css', banner + player.trimEnd() + '\n'); |
There was a problem hiding this comment.
🟡 build.mjs writes a granular dist/css/player.css copy (mirroring tokens/foundation/components), but package.json's exports map never exposes it — only ./player.css -> ./dist/player.css (the stamped standalone) is public. Unlike the other three cascade layers, this granular copy has no consumer entry point, so it's dead build output every run; either add a ./css/player.css export or drop the write.
Extended reasoning...
build.mjs (lines 29-32) builds four granular per-layer copies into dist/css/: tokens.css, foundation.css, components.css (written in the loop at lines 24-27), and player.css (written explicitly at line 31). package.json's exports map exposes ./tokens.css, ./foundation.css, and ./components.css as public subpaths pointing at their respective dist/css/*.css files — but ./player.css is mapped to ./dist/player.css, the separately-stamped standalone copy written on line 32, not to dist/css/player.css.
The result: dist/css/player.css is written on every build but has no export subpath, no README reference, and no example usage anywhere in the repo. Because package.json declares an exports field, Node (and bundlers that respect it) block any deep import path that isn't explicitly listed — so a consumer literally cannot reach dist/css/player.css even by hand-authoring the path. It is unreachable dead output.
This is asymmetric with the other three cascade layers, each of which has both a granular dist/css/*.css copy AND a corresponding export subpath for that same file. player.css is the only layer where the granular copy and the public entry point diverge — one file is exported (the stamped standalone dist/player.css) while a different, functionally-redundant file (dist/css/player.css) is silently produced and thrown away.
Proof/reproduction:
- Run
npm run build. - Observe
dist/css/player.cssexists on disk (written bybuild.mjs:31). - Check
package.json'sexportsmap: no key resolves to./dist/css/player.css. - Try
import '@ceeblue/web-ui/css/player.css'(or any subpath pointing at that file) from a consumer — Node's exports-map encapsulation rejects it, since only the listed subpaths are resolvable. - Meanwhile
import '@ceeblue/web-ui/tokens.css'resolves fine, because./tokens.cssis mapped to./dist/css/tokens.cssin the exports map — confirming the pattern the other three layers follow butplayer.cssbreaks.
Fix options: either add a ./css/player.css export subpath pointing at dist/css/player.css for symmetry with the other layers, or simply drop the redundant writeFileSync('dist/css/player.css', player) call on line 31 since only the stamped standalone (dist/player.css) is ever consumed.
This is a build-output/packaging inconsistency, not a correctness bug — nothing breaks, no consumer is affected today since the file isn't imported anywhere. It's a nit: minor wasted build work and a small trap for future maintainers who might assume dist/css/player.css is exported like its siblings.
|
|
||
| let bundle = banner; | ||
| for (const file of BUNDLE) { | ||
| const css = readFileSync('src/styles/' + file, 'utf8'); | ||
| writeFileSync('dist/css/' + file, css); | ||
| bundle += css.trimEnd() + '\n'; | ||
| } | ||
| writeFileSync('dist/styles.css', bundle); | ||
|
|
||
| // player.css: granular copy in css/ + a stamped standalone at dist/player.css. | ||
| const player = readFileSync('src/styles/player.css', 'utf8'); | ||
| writeFileSync('dist/css/player.css', player); | ||
| writeFileSync('dist/player.css', banner + player.trimEnd() + '\n'); | ||
|
|
||
| // Self-hosted fonts: the stylesheet + its woff2 files | ||
| cpSync('src/fonts', 'dist/fonts', { recursive: true }); | ||
| writeFileSync('dist/fonts.css', banner + readFileSync('src/styles/fonts.css', 'utf8').trimEnd() + '\n'); |
There was a problem hiding this comment.
🟡 build.mjs writes dist/css/tokens.css, dist/css/foundation.css, and dist/css/components.css (the exact files exposed by the granular ./tokens.css, ./foundation.css, ./components.css package.json exports) as raw copies with no version banner, unlike dist/styles.css, dist/player.css, and dist/fonts.css which all prepend banner. This contradicts the stated goal ("Every output is stamped...so hand-vendored copies stay identifiable") and leaves consumers of the granular import paths with unidentifiable CSS. A shared stamp(css) => banner + css.trimEnd() + "\n" helper applied to every writeFileSync call would fix this and remove the current triplicated banner-concatenation logic.
Extended reasoning...
The bug: build.mjs promises, in its own header comment (line 11) and in the README, that "Every output is stamped with a version banner so hand-vendored copies stay identifiable." In practice, three of the seven files written to dist/ are not stamped:
dist/css/tokens.css,dist/css/foundation.css,dist/css/components.css— written atwriteFileSync("dist/css/" + file, css)inside theBUNDLEloop, using the rawcssread straight fromsrc/styles/*.css.dist/css/player.css— written atwriteFileSync("dist/css/player.css", player), also raw.
Compare this to the three outputs that do get stamped: dist/styles.css (bundle starts as banner and each file's content is appended), dist/player.css (banner + player.trimEnd() + "\n"), and dist/fonts.css (banner + readFileSync(...).trimEnd() + "\n").
Why it matters: This is not a cosmetic edge case — the unstamped files are exactly the ones exposed by the granular import paths that the README advertises as a first-class usage pattern:
./tokens.css -> ./dist/css/tokens.css
./foundation.css -> ./dist/css/foundation.css
./components.css -> ./dist/css/components.css
A consumer who imports @ceeblue/web-ui/tokens.css granularly (as the README explicitly documents and encourages, e.g. @import '@ceeblue/web-ui/tokens.css';) receives a file with zero indication of which package version produced it. If they hand-vendor a copy of that file (copy-pasting it into their own repo, which is the exact scenario the banner exists to protect against), there is no way to trace it back to a version later. Note ./player.css is not affected the same way — it resolves to the stamped dist/player.css, not the raw dist/css/player.css — so only the three bundle-layer granular exports are impacted for imports, though the raw dist/css/player.css file is still written unstamped and sitting in the published package.
Why nothing catches this today: There's no test or lint rule asserting dist output contents contain the banner, and the loop that writes the granular per-layer files (dist/css/*.css) is structurally separate from the bundle-accumulation logic (bundle += css.trimEnd() + "\n") that happens to also prepend the banner — so the divergence is easy to introduce and easy to miss in review.
Step-by-step proof:
- Run
npm run build(or tracebuild.mjslines 20-36 by hand). versionresolves to e.g.1.0.0, sobanner = "/*! @ceeblue/web-ui v1.0.0 | AGPL-3.0-or-later */\n".- For
file = "tokens.css":css = readFileSync("src/styles/tokens.css"); thenwriteFileSync("dist/css/tokens.css", css)— the banner is never concatenated ontocssbefore this write.dist/css/tokens.csson disk is byte-for-byte the source file, no banner line. - Meanwhile
bundle(used only fordist/styles.css) does get the banner, becausebundlewas initialized asbannerat the top and each file's content is appended to it — but that assembled string is never written back todist/css/tokens.css. - Same pattern at line 31:
player = readFileSync("src/styles/player.css");writeFileSync("dist/css/player.css", player)— raw, no banner — while the sibling call two lines later,writeFileSync("dist/player.css", banner + player.trimEnd() + "\n"), does get it. - Cross-check
package.json'sexportsmap:"./tokens.css": "./dist/css/tokens.css","./foundation.css": "./dist/css/foundation.css","./components.css": "./dist/css/components.css"— confirming these unstamped files are the actual resolution target for the documented granular import paths, not just internal build artifacts.
Fix: introduce one helper, e.g. const stamp = css => banner + css.trimEnd() + "\n";, and use it for every writeFileSync call that produces a shipped .css file (the BUNDLE loop's dist/css/*.css writes, dist/css/player.css, and the existing three). This closes the gap and eliminates the duplicated banner-concatenation expression that appears three times today.
| @media (prefers-color-scheme: dark) { | ||
| :root:not([data-cb-theme="light"]) { | ||
| --cb-bg: #0f1622; | ||
| --cb-bg-s: #161e2d; | ||
| --cb-bg-e: #1d2638; | ||
| --cb-bg-h: #283248; | ||
| --cb-border: rgb(255 255 255 / 8%); | ||
| --cb-border-s: rgb(255 255 255 / 18%); | ||
| --cb-txt: #e7ecf3; | ||
| --cb-txt-2: #aab4c8; | ||
| --cb-txt-m: #6c7894; | ||
| --cb-navy: #fff; | ||
| --cb-shadow: 0 4px 24px rgb(0 0 0 / 45%); | ||
| --cb-shadow-modal: 0 24px 64px rgb(0 0 0 / 60%); | ||
| --cb-modal-back: rgb(0 0 0 / 55%); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 tokens.css duplicates the entire dark-palette declaration list verbatim in two places — the @media (prefers-color-scheme: dark) block and the :root[data-cb-theme="dark"] block (lines ~62-92) — so any future dark-color tweak (e.g. --cb-bg, --cb-txt) must be applied in both spots or the OS-driven and explicit-override dark modes silently diverge. Consider hoisting the shared values into a single declaration set (e.g. via light-dark() or a shared selector list) so there's one source of truth.
Extended reasoning...
What the bug is
src/styles/tokens.css defines the same 13 dark-theme custom properties twice, with identical values, in two separate rule blocks:
@media (prefers-color-scheme: dark) { :root:not([data-cb-theme="light"]) { ... } }(lines 61-75):root[data-cb-theme="dark"] { ... }(lines 78-92)
Both blocks declare --cb-bg, --cb-bg-s, --cb-bg-e, --cb-bg-h, --cb-border, --cb-border-s, --cb-txt, --cb-txt-2, --cb-txt-m, --cb-navy, --cb-shadow, --cb-shadow-modal, and --cb-modal-back with byte-for-byte identical values.
Why this exists / how it manifests
CSS media queries cannot be combined with attribute-selector rules into a single rule body — @media (prefers-color-scheme: dark) only ever wraps selectors that should apply when the OS prefers dark, while [data-cb-theme="dark"] is the explicit override a consuming app sets via JS. Because these are two independent trigger conditions, the author (reasonably) wrote out the same value block twice rather than trying to force them into one selector.
Why nothing currently prevents this from drifting
There is no lint rule, build step, or code-review gate that checks the two blocks stay in sync — stylelint validates CSS syntax/style, not cross-rule value equality. Nothing stops a future PR from updating just one of the two blocks.
Concrete proof-of-drift scenario
- A designer requests a slightly darker
--cb-bgfor the dark preferred-color-scheme case only (say, to reduce OLED burn-in on displays that never explicitly opt into dark viadata-cb-theme). - A contributor edits
--cb-bg: #0f1622;inside the@media (prefers-color-scheme: dark)block only, and ships it. - Now, a user whose OS is set to dark and who never sets
data-cb-themegets the new value. But an app that explicitly sets<html data-cb-theme="dark">(per the README’s own documented usage) still renders the old--cb-bg. - The two "dark mode" experiences the design system explicitly documents (
README.md: "Dark follows the OS via prefers-color-scheme, and<html data-cb-theme="light|dark">always wins over both") have now silently diverged, and nothing in CI catches it.
Suggested fix
Since a media-query rule and an attribute-selector rule cannot be merged into one CSS rule, true deduplication requires either:
- Using the (baseline-supported)
light-dark()CSS function withcolor-schemeset per selector, so each token is defined once, or - Extracting the raw color literals into an intermediate token layer (e.g.
--cb-dark-bg: #0f1622;etc., defined once) and having both blocks simply assign--cb-bg: var(--cb-dark-bg);— one visible duplication (the assignment lines) but a single source of truth for the actual literals.
Either approach turns "must edit 13 values in 2 places" into "edit 13 values in 1 place."
Severity
This is a pure maintainability/DRY concern — the current code is functionally correct as shipped, nothing breaks today, and there is no user-facing bug. It only creates risk for future edits. Per the review rubric, code-quality/duplication cleanups like this are nit, not blocking.
There was a problem hiding this comment.
The problem with light-dark is that it's relatively recent
Poppins and JetBrains Mono are third-party fonts under SIL OFL 1.1, not under this project's AGPL. OFL clause 2 requires every redistributed copy to carry the copyright notice and licence, and dist/fonts/ shipped six woff2 files with neither. Add each font's OFL text (the copyright holders differ) plus a provenance README covering versions, subsets and how to refresh the files. build.mjs already copies src/fonts recursively, so the notices now travel into the npm tarball unchanged.
player.css moves from `ceeblue.components` into its own `ceeblue.player` layer, declared after components. Sharing a layer left the override order to import order plus specificity, so "import player.css last" was load-bearing but unenforceable; the dedicated layer states it, and turns a later extraction into @ceeblue/player-ui into a file move rather than a cascade change. .cb-btn-accent moves the other way, into components.css: it is a generic variant rather than player chrome, and videojs-plugins already uses it twice — which is exactly the promotion rule player.css states for itself. The .cb-sel caret was three copies of one data-URI SVG, two of them hard-coding --cb-txt-m by hand, so a token tweak silently desynced it. A data URI cannot read var() and a <select> takes neither pseudo-elements nor an element-wide mask, so it is now drawn with two gradients straight from the token: one rule, no per-theme copies. It renders as a solid arrow instead of a stroked chevron.
Document the ceeblue.player layer in the order and in the composition example, and state when the player kit graduates to its own package, so the question does not have to be relitigated: a second kit needing the same treatment, or a release cadence diverging from the design system's. Add a Browser support section. The floor is Chrome 111 / Safari 16.2 / Firefox 121, set by color-mix() and :has(), and it is worth stating explicitly because color-mix() does not degrade — below it the affected backgrounds resolve to transparent rather than to a fallback tint.
2 participants
Ceeblue Web UI — initial design system
Factorizes the CSS that previously lived inline in
wrts-client'splayer.htmlinto a standalone, publishable design system that every Ceeblue web app can share.
Pure CSS — no DOM, no JavaScript.
Merging this publishes
@ceeblue/web-ui@1.0.0(the registry currently holdsonly the
0.0.1bootstrap version used to configure npm trusted publishing).Entry points
@ceeblue/web-ui/…/styles.css…/tokens.css…/foundation.css…/components.css…/player.css…/fonts.cssdist/fonts/*.woff2Every output is stamped with a
/*! @ceeblue/web-ui vX.Y.Z */banner sohand-vendored copies stay identifiable.
Cascade layers
ceeblue.tokens<ceeblue.foundation<ceeblue.components, so a consuming appoverrides anything without specificity hacks:
Everything is
cb--prefixed — tokens (--cb-accent,--cb-bg) and classes(
.cb-btn,.cb-modal-*) — to avoid collisions with the host app.Theming
The light palette sits on bare
:root, so tokens resolve before any JS runs — noflash of unstyled content. Dark follows the OS via
prefers-color-scheme, and<html data-cb-theme="light|dark">always wins over both.Opt-in player kit
The Player-specific classes (
.cb-net-row,.cb-ctrl-row,.cb-stats-actions, …)ship as a separate
player.cssrather than in the bundle, so apps that aren't theplayer don't inherit a buffer-tuning grid they'll never render.
Self-hosted fonts
fonts.cssprovides the families referenced by--cb-f-bodyand--cb-f-mono:Poppins 400/500/600/700 and JetBrains Mono 400/500 (latin subset, both OFL).
Skip the import and the tokens fall back to
system-ui/ui-monospace.Styleguide
examples/index.htmlis a kitchen-sink page rendering every token and component— color tokens, typography, radii, buttons, form controls, toggle, tabs, message
log, modal, plus the opt-in player kit — with a light/dark/system switcher. It
links
/dist, so it doubles as a visual check of what actually ships.Tooling
npm run lint), lint-staged on commit, build on pushlint.ymlon push + PR;release.ymlruns semantic-release with npm trustedpublishing (OIDC, no token) and SLSA provenance, matching
web-utilsRelationship to
web-utilsNo dependency in either direction. The
web-utilswidgets self-host their stylesand reference
--cb-*tokens by name, so they render standalone and pick up thistheme when it's loaded. An app composes both.