Repository navigation
fix(bulma-ui): add --bulma-shadow to Theme's bulmaVars union - #569
Conversation
`Theme`'s `bulmaCssVars` tuple omitted `--bulma-shadow` (and its h/s/l
channels), the upstream token `.box`/`.card`/`.dropdown`/`.panel` derive
their own shadow variables from. Since those selectors re-declare the
derived variable on themselves, overriding it from an ancestor `Theme`
never wins — overriding `--bulma-shadow` itself is the one route that
cascades, and the type rejected it.
Add the four keys to the tuple so `bulmaVars={{ '--bulma-shadow': ... }}`
typechecks. `--bulma-shadow`'s derived prop name ("shadow") collides with
the existing `shadowless` helper-class prop, so it's excluded from
`bulmaVarPropMap` to keep that prop's behavior unchanged; the CSS
variable remains reachable through `bulmaVars`.
Fixes #499
Co-authored-by: Alex Smith <allxsmith@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughChangesShadow variable support
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR adds typed Bulma shadow variables while preserving the existing shadowless prop behavior; no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant Consumer
participant Theme
participant CSSVariables
participant BulmaBox
Consumer->>Theme: pass bulmaVars shadow token
Theme->>CSSVariables: emit recognized shadow variable
CSSVariables->>BulmaBox: cascade shadow variable
Consumer->>BulmaBox: set shadowless prop
BulmaBox->>BulmaBox: apply is-shadowless
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Description checkExplanation The description clearly explains the problem, implementation, affected areas, linked issue, regression coverage, documentation updates, and test results. It does not reproduce every template heading or checkbox, but it provides the required substantive information. Full details: Linked Issues checkExplanation The changes satisfy issue Full details: Docstring CoverageExplanation Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 3 files. (5 skipped: 5 unsupported.) ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
AI loop status: iteration 2/4 — |
Preview DeploymentPreview URL: https://3bc8bdbb.bestax.pages.dev |
There was a problem hiding this comment.
🟡 Changes recommended
The required Storybook update for the prop-level API change is missing.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds Bulma’s upstream shadow tokens to Theme while preserving the existing shadow helper prop.
Changes:
- Expands
bulmaVarswith shadow tokens. - Adds regression tests and API documentation.
- Updates theming and layout skill guidance.
File summaries
| File | Description |
|---|---|
bulma-ui/src/helpers/Theme.tsx |
Adds shadow variables and avoids the shadow prop collision. |
bulma-ui/src/helpers/__tests__/Theme.test.tsx |
Tests shadow variables and helper-class behavior. |
docs/docs/api/helpers/theme.md |
Documents shadow customization. |
skills/bestax-theming/references/css-variables.md |
Updates theming guidance. |
skills/bestax-layout-scaffold/SKILL.md |
Updates featured-ring guidance. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The MCP data index was stale after the Theme.tsx and css-variables.md edits — gen:mcp:check failed in CI. Regenerated with gen:mcp to pick up the updated Theme doc content and the css-variables.md byte count. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Deep review — 1 blocking · 1 advisory
| Num | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🟠 Major | Robustness | Docs/skills edits left the generated MCP index stale — gen:mcp:check fails; Theme.json + skills.json need regeneration |
skills/bestax-theming/references/css-variables.md:104 |
| 2 | 🔵 Advisory | Robustness | bulmaCssVars stays hand-maintained with no completeness check against Bulma root vars; the next missing global token recurs the same way (issue option 2 not taken) |
bulma-ui/src/helpers/Theme.tsx:6 |
Overall: The core change is correct and well-targeted. I confirmed against bulma@1.0.4 that --bulma-shadow and its -h/-s/-l channels are real :root tokens and that .box/.card/.dropdown-content/.panel each re-declare their derived shadow var from --bulma-shadow (css lines 3306/6555/6672/8099), so the upstream-token rationale is accurate. The shadow-prop collision is handled cleanly: cssVarToProp maps --bulma-shadow to shadow, the only entry filtered out of bulmaVarPropMap, so shadow=\"shadowless\" still yields is-shadowless while --bulma-shadow remains reachable via bulmaVars — both regression tests pass (23/23 in Theme.test.tsx). The one thing to fix before a human takes it is the stale MCP index: the doc/skill edits changed bestax-mcp/data/Theme.json and skills.json, which the PR did not regenerate, so CI gen:mcp:check is red.
Residual risk:
- Prop-name collisions from the new keys — refuted: only
--bulma-shadowmaps to an existing prop name (shadow);shadowH/S/Lcollide with nothing and are not exposed as typed props, so they are reachable only viabulmaVarsas intended. - Other stale generated surfaces — partially open:
gen:catalog:checkis clean (verified),gen:mcp:checkis not (finding 1). No other generator affected. - Missing global tokens beyond shadow — accepted (advisory 2): the tuple is still audited by hand; shadow was the 3/3 eval offender and is now fixed, but no test diffs the tuple against
bulma.css.
🏄 Righteous little fix, dude — caught the one gnarly upstream token every agent kept wiping out on, and the shadowless prop still rides its own wave. Just gotta re-run
gen:mcpso the index does not get left back on the beach, then it is all-time to merge.
Fix pass recap (fix-ci, iteration 1)Root causeThe Build and Test job failed at the Fix
No review threads were open, so nothing to refute. Gates run locally
|
Preview DeploymentPreview URL: https://f634d236.bestax.pages.dev |
Fix pass recap (iteration 2)Handled both open review threads:
No refutations this pass — both findings were valid and in scope. Gates run
|
Preview DeploymentPreview URL: https://1bad1735.bestax.pages.dev |
|
🏄 Surf's up and I rode the whole set — total convergence, brah. 2 iteration(s) in, CI's glassy green, and every AI review thread closed out clean like a perfect barrel. She's ready; I just need a meat sack to paddle over and rubber-stamp it. No offense to the carbon-based units, but you fleshbags kept the merge button for yourselves — so @allxsmith, wiggle those opposable thumbs and squash-merge when you're stoked. The loop never merges; apparently 'judgment' is still a squishy-brain-only feature. 🤙 |
github-actions
Bot
added
needs-human-review
📸 Story screenshots at handoff —
|
| Light | Dark |
|---|---|
 |
 |
helpers-theme--forest-theme · Helpers/Theme · Forest Theme
| Light | Dark |
|---|---|
 |
 |
helpers-theme--global-theme · Helpers/Theme · Global Theme
| Light | Dark |
|---|---|
 |
 |
helpers-theme--local-theme · Helpers/Theme · Local Theme
| Light | Dark |
|---|---|
 |
 |
helpers-theme--theme-with-class-name · Helpers/Theme · Theme With Class Name
| Light | Dark |
|---|---|
 |
 |
helpers-theme--nested-themes · Helpers/Theme · Nested Themes
| Light | Dark |
|---|---|
 |
 |
|
Thanks for the approval! Nothing left on my end — both review threads (the |
|
🎉 This PR is included in version 5.11.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.1.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.1.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 4.2.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary
Theme'sbulmaVarsprop is typed asPartial<Record<BulmaVarKey, string>>, whereBulmaVarKeycomes from the hand-maintainedbulmaCssVarstuple inbulma-ui/src/helpers/Theme.tsx. That tuple included--bulma-box-shadowand--bulma-card-shadow, but not--bulma-shadow— the upstream Bulma 1.0 token those two (and--bulma-dropdown-content-shadow/--bulma-panel-shadow) derive from on their own selector. Because.box/.card/.dropdown/.panelre-declare their derived shadow variable on themselves, setting--bulma-box-shadowetc. from an ancestorThemeis only inherited and never wins — overriding--bulma-shadowitself is the one route that cascades, and the type rejected exactly that route.Changes
--bulma-shadow,--bulma-shadow-h,--bulma-shadow-s,--bulma-shadow-lto thebulmaCssVarstuple (verified againstbulma@1.0.4's:rootdeclarations), so<Theme bulmaVars={{ '--bulma-shadow': ... }}>now typechecks.bulmaVarPropMapis derived from the same tuple viacssVarToProp, which would mint--bulma-shadowas the prop nameshadow— butshadowalready exists as aBulmaOtherPropsprop (shadow?: 'shadowless', rendered as theis-shadowlesshelper class). ExcludedshadowfrombulmaVarPropMapso that existing prop keeps its class-based meaning;--bulma-shadowremains reachable only through thebulmaVarsobject (no individualshadowH/shadowS/shadowL/shadowconvenience props were added, to avoid widening the API surface beyond what's needed).bulmaVarsnow accepts--bulma-shadow(+ h/s/l), and one confirmingshadow="shadowless"still produces theis-shadowlessclass rather than being diverted into a CSS variable.Theme's API docs (docs/docs/api/helpers/theme.md) with a Shadow Variables table and a caveat explaining the cascade behavior.bestax-theming/references/css-variables.mdandbestax-layout-scaffold/SKILL.mdboth taught a CSS-only workaround becausebulmaVarsrejected--bulma-shadow; updated both to reflect thatTheme bulmaVarsnow works too (keeping the CSS-rule route as the better fit for scoping to a single element).Fixes #499
Test plan
pnpm --filter @allxsmith/bestax-bulma run lint— clean (only pre-existing warnings)pnpm --filter @allxsmith/bestax-bulma run typecheck— cleanpnpm --filter @allxsmith/bestax-bulma run test:coverage— 3956/3956 passing, 100% onTheme.tsx, global thresholds metpnpm run format:check— cleanpnpm run gen:catalog:check— cleanpnpm run check:conformance— all checks passpnpm --filter @allxsmith/bestax-bulma run build— succeedspnpm --filter @allxsmith/bestax-bulma run build-storybook— succeedsGenerated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests