ReviewOS

stacks/stx

components: hardcoded palette shades and raw <a href> mean the library cannot follow an app's theme or its router

#1930
Closed glennmichael123 opened this 14 days ago · 2 comments
14 days ago

Components hardcode palette shades and raw <a href>, so they cannot follow an app's theme or its router

Package: @stacksjs/components · Measured at: 0.2.175 and 0.2.176 · Scope: all 91 .stx files in src/ui

Two properties of the shipped components make them hard to adopt in an app that themes with tokens or uses the SPA router. Neither is a bug — both render correctly — but together they mean a component library that visibly does not belong to the app that installs it.

Filing these together because they have the same fix shape: the component should defer a decision to the host app instead of baking it in.

1. 62 of 91 components hardcode Tailwind palette shades

Measured by matching \b(?:bg|text|border|ring|divide)-(?:gray|slate|zinc|neutral|stone|red|green|blue|yellow|indigo|purple|pink)-\d{2,3}\b across src/ui:

count
components with hardcoded palette shades62
components referencing a CSS variable (var(--…))4
components shipping a dark: variant67

Worst offenders by occurrence count: SubscriptionCheckout (91), Login (52), Signup (48), PasswordInput (40), Navigator (37), TextInput (36).

Dark mode is handled — via Tailwind's dark: variant. What is not handled is a host app with its own palette. An app whose colours live in semantic tokens (bg-panel, border-line, text-subtle, or a theme.extend.colors map) installs <Button> and gets bg-gray-100, which matches nothing around it. There is no prop, no CSS variable and no config hook to redirect it.

Concretely, Breadcrumb.stx renders:

<a href="/" class="text-gray-600 dark:text-gray-400 hover:text-gray-900 dark:hover:text-gray-200 …">Home</a>
<span class="text-gray-400 dark:text-gray-600 select-none" aria-hidden="true">/</span>

Suggested direction: emit CSS custom properties with the current palette as the fallback, so an unstyled app looks identical and a themed one can override:

class="text-[color:var(--stx-fg-muted,theme(colors.gray.600))]"

Even a documented list of the variables each family reads would be enough — the blocker is that there is currently no seam at all.

All eight, with anchor counts:

componentfamily<a href>
Loginauth4
Signupauth2
TwoFactorChallengeauth1
SidebarItemsidebar1
SidebarPinnedsidebar1
Navigatornavigator1
Breadcrumbbreadcrumb1
SubscriptionCheckoutpayment1

Components using StxLink: 0.

StxLink is a builtin, registered unconditionally (builtins/index.js), so it is available inside a component with no import and no registration. A raw anchor in an SPA is a full page reload — so adopting <SidebarItem> or <Login> silently opts that part of the app out of client-side navigation.

This is the one that bites hardest, because Sidebar*, Breadcrumb, Navigator and the auth family are exactly the navigation surfaces an app adopts first — and they are precisely where a full page reload is most visible.

Suggested direction: use StxLink for internal targets, falling back to <a> for absolute/external URLs — or take an as prop so the host can supply its own link component.

3. Minor: RadioGroup renders a hydration shell

<RadioGroup :options="[{label:'…',value:'a'}]" /> emits a data-stx-scope wrapper with the labels only inside data-stx-props. Nothing readable is in the server HTML, so the options are invisible without JS and to a crawler.

Measured by stripping every attribute value from the output before searching for the label. Nine other components checked the same way — including Tabs, Accordion, Select, Navigator, Breadcrumb and VirtualTable — do server-render their content, so this looks like an oversight in one component rather than a design choice.

Method

Each figure comes from rendering through processDirectives(template, context, filePath, options, dependencies) with the plugin dir registered, then stripping the injected crosswind <style> block before asserting on the output.

Two notes for anyone reproducing it: processDirectives takes five arguments and omitting dependencies makes every call throw — a probe that only checks for known failure strings will then report clean passes over non-renders. And the injected <style> block is not evidence that a component rendered; assert on markup only that component can emit.

Unrelated good news

The two defects that had 32 of these components quarantined in our internal standards are both fixed as of 0.2.175 — processDynamicComponents now runs before processComponents, and parseLoopBinding accepts the @foreach(x in y) header form. All 36 previously-forbidden tags render cleanly. Thank you.

14 days ago

All three parts are in, across aa43fa7e3f and 4b93bf6a96. Taking them in the order they bite.

2. Raw anchors — done, but four of the twelve were not what they looked like

Your counts were exact: 8 components, 12 anchors, 0 using StxLink.

Breadcrumb, SidebarItem, SidebarPinned, Navigator now use <StxLink>. No as prop and no internal/external branching was needed — the router's own exclusion list already skips http, #, mailto:, tel: and javascript: even on a marked element, so an external to still gets a native navigation. That was worth checking before building a fallback nobody needs.

The other four were href="#", and converting those to <StxLink to="#"> would have been theatre: the router excludes hash targets, so nothing would change. They were not navigation — they were placeholders an app had no way to wire, which is arguably the worse defect:

  • Login — forgotPasswordHref / signupHref props (defaulting to /forgot-password and /register), through StxLink.
  • Login, Signup — the social sign-in anchors are buttons now, emitting social with the provider so an app can start its own OAuth flow.
  • TwoFactorChallenge — the recovery-code link only ever ran a handler. It is a button. A hash href is a real navigation to the top of the page for a keyboard user.
  • SubscriptionCheckout — the product links only when the product has an href, rather than announcing itself to a screen reader as a link and going nowhere.

Two things fell out of doing it:

  • :prop bindings were being dropped by builtins. <StxLink to="/x" :title="pageTitle"> emitted an <a> with no title and no warning — only static was ever forwarded; a server-evaluated binding was resolved, stored, and thrown away. Fixed for StxLink, StxImage and SafeImage. null/undefined/false drop the attribute rather than rendering the word, which is what makes :aria-current="active ? 'page' : null" mean "sometimes".
  • An inline inside a component tag applies to only its first attribute — b="2" leaks out unconditionally. Filed as #1931 with the repro; the parser is high-blast-radius enough that I did not want to change it in the same pass as a component migration. The converted components avoid the shape.

3. RadioGroup — done

It had no options prop at all, which is why the labels only reached the page inside data-stx-props. It now server-renders them, with aria-checked correct without JS.

They are emitted by RadioGroup rather than delegated to <RadioGroupOption> on purpose: a loop variable cannot cross a component boundary as a : binding, and interpolating it would stringify the value — 1 and "1" would stop being distinguishable, and the group compares by identity. Emitting the markup directly keeps data-option-value exact JSON, which is what syncOptionStates already reads; selection is claimed by one delegated listener on the root. The slot API is untouched.

(Separate, untouched: <RadioGroupOption>'s own <slot :disabled="disabled" /> does not fill — a scoped slot on a component emits a literal <slot / />.)

1. Palette — measured first, and the answer changed

Part of this already worked. A project's crosswind.config.ts theme.colors deep-merges over the base palette, and stx generates CSS by scanning the rendered page. So this:

export default { theme: { colors: { gray: { 600: '#3f3f46' } } } } }

…re-themes every component that names gray-600, with no component edits and no opt-in. I verified it against a real render before believing it: .text-gray-600 in Breadcrumb's output comes out as the app's value, and shades you don't name keep their defaults. It was undocumented, which is how it reads as absent. It is now pinned by a test, because a working seam nobody wrote down is one refactor from not existing.

What was genuinely missing is a runtime seam and semantic names, which is what your suggested direction was really after. stxThemePreset (@stacksjs/components/theme) rebuilds the palette so every themed shade resolves through a CSS custom property with today's value as the fallback, and adds role tokens on top — surface, fg-muted, line, accent, danger — each falling back through the shade variable to the stock value:

// crosswind.config.ts
import { stxThemePreset } from '@stacksjs/components/theme'
import { defaultConfig } from '@cwcss/crosswind'
export default { theme: { colors: stxThemePreset(defaultConfig.theme.colors) } }
:root { --stx-color-gray-600: #52525b; --stx-surface: #fbfbfd; }
[data-tenant="acme"] { --stx-accent: #e11d48; }

themeVariableNames() returns the full list, generated from the same source as the palette so it cannot drift — that is the "documented list of the variables each family reads" you said would be enough.

It is opt-in rather than the default because of a real trade: an opacity modifier on a variable-backed colour compiles to color-mix() instead of a baked oklch(… / 0.5). Everything keeps working (asserted — silently generating nothing would delete every bg-black/4 in the library the moment someone adopted it), but the emitted CSS changes and needs a 2023+ browser. That is the app's call, not the framework's.

What I did not do: migrate the components themselves from text-gray-600 to text-fg-muted. Both routes above work regardless, since they redirect the palette those classes resolve through — but you still can't say "every border comes from --stx-line". That is 903 occurrences across 62 files and each one is a judgement about which role the shade is playing, not a substitution. Doing it by search-and-replace would quietly change the appearance of the whole library. docs/features/theming.md states this as a current limitation rather than implying coverage.

On the method notes

Both were right and both cost me time before I read them properly. processDirectives does take five arguments, and the injected <style> really is not evidence of a render — my theming tests initially "passed" against a stock palette because the resolved crosswind config is cached in module state for the life of the process, so the first config in the file was silently answering for every later one. There is a resetCrosswindCache() if you hit the same thing.

14 days ago

The palette half is done — 5b58cb06b6.

Where the tokens live

In stx's base theme, not an opt-in preset. That was the load-bearing decision: a component saying text-fg-muted has to resolve in an app that has never heard of the vocabulary, or adopting the library silently requires a config change and renders unstyled without it. Adding names is purely additive, so no existing utility changed and no opacity modifier on an existing palette colour became color-mix().

Each token is var(--stx-<role>, <light value>), and mergeCrosswindConfig — the one merge both render paths already shared — emits the :root/.dark block. So one class is correct in both modes, and you re-theme by setting a variable at runtime:

:root { --stx-accent: #e11d48; --stx-surface: #fbfbfd; }
.dark { --stx-surface: #18181b; }
[data-tenant="acme"] { --stx-accent: #0ea5e9; }

The light fallback is deliberate: if that block ever fails to reach a page, light mode is still correct and dark mode degrades to the light colour rather than to an invalid one.

The set was measured

Every role's default is the pairing the library already used most — text-gray-900 dark:text-gray-100 ×70 → text-fg, border-gray-200 dark:border-gray-700 ×21 → border-line. Five text roles rather than three, because the library genuinely distinguishes five.

The one that surprised me: accent/danger/info/success needed -solid counterparts. Coloured text must lighten on a dark background to stay legible (600 → 400); a solid fill barely moves (600 → 500). My first pass folded them together, and the migration report proudly described turning every primary button paler in dark mode as a "normalization". Same reason hover states on solid fills are excluded — a button darkens on hover in both modes, while the token's dark value lightens.

Migration: 622 of ~890 uses, 49 files

Applied by table, and only where a light utility had a dark twin in the same class string, so each rewrite replaced a complete pair. Left on explicit shades:

  • a shade with no dark twin — a token would add dark-mode behaviour it never had;
  • a pair whose dark twin is a different hue (text-neutral-900 dark:text-blue-400 is a deliberate colour, not a shade of one role);
  • hover states on solid fills, per above.

43 occurrences used a pairing one step off the dominant one and were normalized onto the role, as you chose.

Also: testDarkModeSupport now counts a role token as dark-mode support. Counting only dark: variants reported a fully migrated component as having none — false, and a test that fails for the wrong reason.

The components preset keeps the palette-level runtime seam and no longer declares roles of its own. It had already drifted from the framework list (it had accent-hover, no fg-strong/info/-solid), which is precisely the failure that module exists to remove.

docs/features/theming.md has the full role table and states what is not migrated rather than implying coverage.

Sign in to comment on this issue.