Repository navigation
fix(bulma-ui): deprecate CSS-less color values, warn in dev, fix has-text fall-through - #491
Conversation
…text fall-through Progress, Notification, and the Hero root accept color values Bulma ships no is-<color> CSS for; Pagination and Tabs ship no color CSS at all. Those values now carry TSDoc deprecation notes and a warn-once dev console warning ahead of removal in the next major. The nine components whose color prop falls through to has-text-<color> (Box, Block, Content, Delete, IconText, Image, Buttons, Card, Container) now document that behavior in TSDoc and give textColor precedence when both props are set.
|
Warning Review limit reached
Next review available in: 41 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
WalkthroughThe pull request standardizes ChangesColor behavior alignment
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Preview DeploymentPreview URL: https://57ed6da9.bestax.pages.dev |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 53 out of 53 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
docs/docs/guides/helpers/color.md:161
- This second
Boxexample also uses the omittedbackgroundColorprop rather than the publicbgColoralias, leaving the live TypeScript example invalid.
docs/docs/guides/helpers/color.md:157 BoxPropsomitsbackgroundColorand exposes this helper asbgColor, so this revised example still fails TypeScript when copied. Use the public prop name here.
This issue also appears on line 161 of the same file.
docs/docs/guides/helpers/color.md:165
NotificationPropsexposes neitherbackgroundColornorbgColor; its supported surface color is the CSS-backedcolormodifier. As written, this revised example does not type-check, so usecolor="success"for the notification background while retainingtextColor.
docs/docs/guides/helpers/color.md:99- The example now uses
textColor, but the same section still tells readers thatcoloruniversally produceshas-text-*and lists every helper value undercolor. That is false for modifier components (and now-deprecated Pagination/Tabs), which is the distinction this PR is intended to clarify. Update the surrounding Text Color prose and tables to documenttextColoras the public helper and reservecolorfor component-specific behavior.
| // `process` may not exist for CDN/no-bundler consumers; treat that as dev. | ||
| // Read via globalThis because the library tsconfig has no Node types. | ||
| const isDev = (): boolean => { | ||
| const env = (globalThis as { process?: { env?: { NODE_ENV?: string } } }) | ||
| .process?.env; | ||
| return env?.NODE_ENV !== 'production'; |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Deep review — 0 blocking · 1 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Robustness | Dev-gate reads globalThis.process dynamically, so it isn't statically replaced; in bundled browser prod builds where process is undefined at runtime (Vite, webpack 5 without a polyfill) isDev() returns true, so a deprecation warning can fire in production when a deprecated value/prop is actually used — contradicting the "production builds are silent" claim. |
bulma-ui/src/helpers/colorDeprecations.ts:31 |
Overall: The change is sound and non-breaking. I chased #367 to source, confirmed the failure shape (wide validColors unions emitting dead is-grey*/is-*-bis/is-*-ter modifiers, plus color silently overriding textColor on the fall-through elements), and verified the fix empirically — 410 tests across the 16 touched suites pass, the warn-once/dev-gate helper is well covered, and the textColor ?? color precedence fix behaves as documented (identical output when one prop is set, textColor wins when both are). The riskiest part is the dev/prod gate (finding #1); the maintainer should decide whether the Vite-prod warning leak is acceptable given the deliberate CDN-safety trade-off.
Residual risk:
- Other root
is-<color>emitters with the same over-wide union: refuted — grepped every component emittingis-${color};Button,Tag,Message,InputBase,SelectBase,Panel,Steps,Badgeall already restrictcolorto the 10 CSS-backed values.Progress,Notification, andHerowere the only ones accepting the full 17-value union, and all three are now handled. - Whole-prop-dead beyond Pagination/Tabs: not exhaustively re-audited, but out of #367's stated scope; the two known cases are covered and warn once in dev.
- De-exempting
Notification/Progressstories fromstories-conformance: refuted — every remaining argType in both story metas now carries adescription, so the conformance test stays green.
🏄 Righteous cleanup, dude — this PR doesn't rip out any types or break the lineup, it just paddles out and honestly tells everyone which colors actually catch a wave and which ones wipe out silently. One tiny ripple: the "quiet in prod" promise might splash a warning in a Vite bundle, but nothing's gonna hold you back from merging. Good to go. 🌊
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bulma-ui/src/helpers/colorDeprecations.ts`:
- Around line 29-35: Update isDev in bulma-ui/src/helpers/colorDeprecations.ts
(lines 29-35) to use the supported build-time development flag or injected
environment predicate, defaulting unknown or missing runtime information to
false so warnings remain disabled. In
bulma-ui/src/helpers/__tests__/colorDeprecations.test.ts (lines 112-127), change
the missing-process case to assert no warning and add coverage confirming
warnings are enabled through the supported development flag.
In `@docs/docs/guides/helpers/color.md`:
- Around line 75-82: Update the color guidance to reflect the component-specific
API: use textColor for generic text-color examples and claims, remove or revise
the Button color examples because Button maps color to the filled button
variant, and scope remaining color alias documentation and examples to
components that implement it, including the affected content around the Span
examples.
- Around line 157-165: Update the two Box examples to use the supported bgColor
prop instead of backgroundColor. Verify Notification’s public prop API and
retain backgroundColor only if it is supported; otherwise replace it with the
correct prop so all examples compile.
In `@skills/bestax-layout-scaffold/references/layout-components.md`:
- Line 63: Update the Hero component’s color documentation table entry to
enumerate the exact CSS-backed Bulma color values and the complete
accepted-but-deprecated set, explicitly including inherit and current, instead
of referring generically to “Bulma color.”
In `@skills/bestax-theming/references/themeable-components.md`:
- Line 50: Update the Notification, Hero, and Progress inventory rows to
explicitly list black-bis, black-ter, and every deprecated grey color alias as
deprecated, while preserving the existing unsupported-value and removal-status
details.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cedcf1c5-5b5c-4f01-9e7f-5d7218dbf8a3
📒 Files selected for processing (53)
bulma-ui/src/__tests__/stories-conformance.test.tsbulma-ui/src/components/Card.tsxbulma-ui/src/components/Pagination.stories.tsxbulma-ui/src/components/Pagination.tsxbulma-ui/src/components/Tabs.stories.tsxbulma-ui/src/components/Tabs.tsxbulma-ui/src/components/__tests__/Card.test.tsxbulma-ui/src/components/__tests__/Pagination.test.tsxbulma-ui/src/components/__tests__/Tabs.test.tsxbulma-ui/src/elements/Block.tsxbulma-ui/src/elements/Box.tsxbulma-ui/src/elements/Buttons.tsxbulma-ui/src/elements/Content.tsxbulma-ui/src/elements/Delete.tsxbulma-ui/src/elements/IconText.tsxbulma-ui/src/elements/Image.tsxbulma-ui/src/elements/Notification.stories.tsxbulma-ui/src/elements/Notification.tsxbulma-ui/src/elements/Progress.stories.tsxbulma-ui/src/elements/Progress.tsxbulma-ui/src/elements/__tests__/Block.test.tsxbulma-ui/src/elements/__tests__/Box.test.tsxbulma-ui/src/elements/__tests__/Buttons.test.tsxbulma-ui/src/elements/__tests__/Content.test.tsxbulma-ui/src/elements/__tests__/Delete.test.tsxbulma-ui/src/elements/__tests__/IconText.test.tsxbulma-ui/src/elements/__tests__/Image.test.tsxbulma-ui/src/elements/__tests__/Notification.test.tsxbulma-ui/src/elements/__tests__/Progress.test.tsxbulma-ui/src/helpers/__tests__/colorDeprecations.test.tsbulma-ui/src/helpers/colorDeprecations.tsbulma-ui/src/layout/Container.tsxbulma-ui/src/layout/Hero.tsxbulma-ui/src/layout/__tests__/Container.test.tsxbulma-ui/src/layout/__tests__/Hero.test.tsxdocs/docs/api/components/card.mddocs/docs/api/components/pagination.mddocs/docs/api/components/tabs.mddocs/docs/api/elements/block.mddocs/docs/api/elements/box.mddocs/docs/api/elements/buttons.mddocs/docs/api/elements/content.mddocs/docs/api/elements/delete.mddocs/docs/api/elements/icontext.mddocs/docs/api/elements/image.mddocs/docs/api/elements/notification.mddocs/docs/api/elements/progress.mddocs/docs/api/helpers/valid-values.mddocs/docs/api/layout/container.mddocs/docs/api/layout/hero.mddocs/docs/guides/helpers/color.mdskills/bestax-layout-scaffold/references/layout-components.mdskills/bestax-theming/references/themeable-components.md
💤 Files with no reviewable changes (3)
- bulma-ui/src/components/Pagination.stories.tsx
- bulma-ui/src/components/Tabs.stories.tsx
- bulma-ui/src/tests/stories-conformance.test.ts
…e to real props The dev-warning gate now reads a bare process.env.NODE_ENV in a try/catch: bundlers replace it statically, and a runtime without process stays silent instead of warning in production. The color guide teaches textColor/bgColor (Title/SubTitle never had a color prop; Box/Card expose bgColor, and Notification has no background prop at all), and the skill inventories name black-bis/black-ter and Hero's inherit/current among the deprecated values.
|
Addressed all review findings in 117c0c0:
|
Preview DeploymentPreview URL: https://2b1d244e.bestax.pages.dev |
|
🎉 This PR is included in version 5.8.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 4.0.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.0.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Closes #367 (the type-level narrowing itself is deferred to #490 — see below).
Bulma 1.0.4 iterates its 11-color
$colorsmap for component modifiers, so several of our color props accept values that emit a class no shipped CSS rule matches. This PR makes every such case visible without breaking anyone, in three moves:1. Dead
is-<color>values now warn in dev and are documented as deprecatedProgress,Notification(both the component prop andNotificationOptionsfor the programmatic API), and theHeroroot accept the 17-valuevalidColorsunion, but onlyprimary, link, info, success, warning, danger, black, white, light, darkhave shipped CSS for.progress/.notification/.hero. The seven grey/bis/ter values (plusinherit/currenton the Hero root) render unstyled — the exact failure that produced the skill-loop eval's only shipped defect (run i10's silently unstyled comparison bar)..d.tssee the truth inline.bulma-ui/src/helpers/colorDeprecations.ts, not exported from the package root) logs one development-mode console warning per component+value. Production builds are silent.2.
PaginationandTabs: the wholecolorprop is deprecatedBoth emit
is-${color}on their root, and Bulma ships no.pagination.is-*or.tabs.is-*color CSS at all — every value has always been a silent no-op (the prop is consumed beforeuseBulmaClasses, so there is nohas-text-*fallback either). The prop now carries an@deprecatedTSDoc tag (rendered as a Deprecated. note in the docs tables) and warns once in dev. Emission is kept for now; removal lands with the follow-up major.3. The
has-text-*fall-through on nine components is now deliberateBox,Block,Content,Delete,IconText,Image,Buttons,Card, andContainerdeclare a 6-valuecolorthat was never destructured — it rode the props spread intouseBulmaClasses, renderedhas-text-<color>, and (because the spread came last) silently overrodetextColorwhen both were set. Now:coloris destructured and passed astextColor ?? color: identical output when one prop is set, andtextColorwins when both are (the bug fix).colorprop states the real behavior: a text-color alias, not a filled variant.Ripples
Progress/Notificationcolor argTypes trimmed to the 10 live values and every argType given adescription; both files removed from the stories-conformanceLEGACY_EXEMPTlist. The PaginationColorsstory (showcasing the dead prop) is deleted; the TabscolorargType is removed.valid-values.md(the "also componentcolorprops" claim), the color guide (<Box color="white">→textColor,<Notification color="white">→textColor, native<span color>→<Span textColor>), and the Pagination/Tabs overview prose.bestax-theming/references/themeable-components.mdwarning block and the Notification/Hero/Progress/Box/Pagination/Tabs rows updated to the deprecated + dev-warn reality;bestax-layout-scaffoldHero row annotated.notification.show()is covered,Hero.Body(a text helper) does not warn, and the nine fall-through components renderhas-text-*withtextColortaking precedence.Non-breaking throughout: no type changed, no prop removed, no emission changed. The only behavior change is the
textColor-precedence bug fix in item 3. Release-wise this is afix(bulma-ui)patch.Summary by CodeRabbit
colornow consistently works as a text-color alias across supported components, withtextColortaking precedence.textColor/bgColoralternatives.