Repository navigation
feat(eslint-plugin): check the gap helper props, with Theme's columnGap as the helper - #878
Conversation
Theme mints a camelCase prop for every Bulma variable, so `columnGap` there was the untyped prop for `--bulma-column-gap`, and ThemeProps left the column gap helper out. It is the column gap helper on Theme now too, joining `radius` and `shadow` in `helperPropNames`. A string that is not a gap step still writes the variable, with a development warning pointing at `bulmaVars`, the way #694 kept the old `radius` route. The one behaviour change is that an untyped JavaScript `<Theme columnGap="0">`, which zeroed the columns gutter inside the Theme, now adds `is-column-gap-0` to the wrapper instead. A gap step on an isRoot Theme does nothing, and warns in development. The Theme API page documents both. The ESLint plugin declares the variable route in its `DEPRECATED_VARIABLE_ROUTE` alongside this, because `scripts/helper-prop-collisions.test.mjs` holds the two to each other. Part of #769.
`valid-helper-value` checks `gap`, `columnGap` and `rowGap` against the library's `validGaps`,
so `<Box gap="1rem" />` is reported. These props take a step as a number as well, so
`NUMERIC_STEPS` keeps `<Grid gap={3} />` and `<Box gap={1.5} />` from being reported, and a
number that is no step gets a message of its own rather than advice to add quotes.
`columnGap` on Theme joins `DEPRECATED_VARIABLE_ROUTE`. A string that is not a gap step still
sets `--bulma-column-gap` there, so the report says so and points at `bulmaVars`.
Part of #769.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 21 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (9)
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://94c4b141.bestax.pages.dev |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Deep review — 2 blocking · 3 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🟡 Minor | Correctness | A three-character CSS length on Theme columnGap is a "near miss" of a gap step, so the report says "Did you mean 8?" instead of pointing at bulmaVars — the one thing the deprecated-route branch exists to say |
eslint-plugin/src/lib/values.ts:273 |
| 2 | 🟡 Minor | Correctness | On Columns, whose own gap is the gutter on a whole-step scale, the report prints 0.5/1.5/… as valid values that render nothing there |
eslint-plugin/src/lib/values.ts:176 |
| 3 | 🔵 Advisory | Correctness | "the class is never emitted" is literally false on Grid and Columns: both build their gap classes unvalidated, so <Grid gap="1rem" /> does emit is-gap-1rem. The conclusion ("nothing renders") still holds, so the advice is right |
eslint-plugin/src/rules/valid-helper-value.ts:99 |
| 4 | 🔵 Advisory | API | columnGap="0" is the one value that genuinely changes behaviour for untyped JS: it was a valid length, and under isRoot it zeroed the gutters page-wide, not just inside the Theme. The PR flags this for you — worth confirming deliberately |
bulma-ui/src/helpers/Theme.tsx:972 |
| 5 | 🔵 Advisory | Coverage | The number branch of the root warning runs but its message is never asserted: warnOnce has already fired on the "3" string by the time columnGap={2} renders |
bulma-ui/src/helpers/__tests__/Theme.test.tsx:703 |
Overall: The bulma-ui half is the strong half — themeColumnGapVar mirrors themeRadiusVar exactly, the helperPropNames entry keeps scripts/helper-prop-collisions.test.mjs honest (it passes against the built plugin here), precedence over bulmaVars is preserved and tested, and the deprecated string route survives so nothing that worked stops working except gap steps. The riskiest part is the ESLint side, for a reason the diff half-anticipates: HELPER_VALUES is keyed by prop, and gap is the first entry whose meaning genuinely differs per element (Columns gutter, whole steps, is-<n>) and whose tuple is a numeric scale short enough that every CSS length looks like a typo of it. Both blocking findings are instances of that; the NUMERIC_STEPS comment reasons about staying silent on Columns, but not about what the rule says when it does speak. Look at finding 1 first — it is the one that sends a migrating user to a wrong value.
Residual risk:
- Other elements with a
gap-shaped prop of their own. Greppedbulma-ui/src: onlyGridandColumnsdeclare one.Grid's three props are typedBulmaGapStepand renderis-gap-*/is-column-gap-*/is-row-gap-*, identical to the helpers, so the table is right there.Columnsis the mismatch — posted as finding 2. gapemitted without a flex/griddisplay. Refuted:useSpacingClasses(bulma-ui/src/helpers/useSpacingClasses.tsx:144-151) emitsis-gap-*with no display gate, unlikeuseFlexboxClasses, sono-inert-flex-propsis not silently under-reporting. The class is emitted; whether CSS gap does anything is a stylesheet fact, not a dropped class.- A second Theme variable colliding with a gap helper. Refuted:
bulmaCssVarsholds--bulma-column-gap,--bulma-grid-gap,--bulma-burger-gapand--bulma-skeleton-lines-gap— no--bulma-gapand no--bulma-row-gap— sogapandrowGapmint no variable prop and need nohelperPropNamesentry. The collisions guard asserts this from source and passes. - Rule and library disagreeing on a number. Refuted by reading both: the rule compares
String(numeric)against the tuple andgapValuedoesString(value), so{0},{-0},{8.0}and{1e1}resolve the same on both sides.numericValuehandles the unary-minus shape, socolumnGap={-1}is reported rather than skipped. gap/rowGap/gaplessunderisRootget no "this does nothing" warning, wherecolumnGapnow does. Deliberate and stated in the comment (onlycolumnGapreads like the gutter it used to set), and consistent with every other helper prop at the root. Open, but not a defect.- Generated artefacts and gates, run on this checkout:
gen:eslint-meta:check,gen:mcp:check,gen:catalog:checkandgen:api-docsall clean; bulma-ui 155 suites / 5491 tests plus the 99% coverage gate pass; eslint-plugin 251 tests plus coverage pass; thescripts/suite passes with all 38 measured files at or above their floors, includinghelper-prop-collisionsandeslint-plugin-docs.check:conformancefails only onversion-regression, because this checkout's tags are unreachable from HEAD — a shallow-clone artefact, nothing to do with the diff.
🏄 Clean swell on the library side —
columnGappaddles out and joinsradiusandshadowon the helper lineup without wiping anyone out. Just watch the inside section on the lint rule: a numeric gap scale makes every8pxlook like an8, andColumnsis surfing a totally different break. Patch those two and it is all-time.
…as lengths Three things `valid-helper-value` said about the gap props were wrong. - A number with a unit got a near miss. "8px" is two edits from the bare step `8`, so `<Theme columnGap="8px">` was asked "did you mean `8`?", a 4rem gap, instead of being given the `bulmaVars` remedy its deprecated route exists for. A value like that is a CSS length, not a mistyped step, so it gets no suggestions. "1.25" still suggests `1.5`, and "4by1" still suggests its neighbouring ratios. - On Columns, whose own `gap` is the whole-step gutter, a report listed the half steps as valid and a half step passed although it renders nothing there. A new `ELEMENT_VALUES` table, read before `HELPER_VALUES`, gives Columns' `gap` the whole steps, read off `validGaps`. - The messages said the class is never emitted. Grid and Columns build their gap classes without checking them, so `<Grid gap="1rem">` does emit `is-gap-1rem`; what is true everywhere is that nothing renders, so the messages say only that. Part of #769.
The isRoot `columnGap` test rendered a step written as a number, but `warnOnce` had already fired for the string case, so that branch's message was never checked. The test resets the dev warnings and asserts it. Part of #769.
Preview DeploymentPreview URL: https://3bdc12dd.bestax.pages.dev |
There was a problem hiding this comment.
Deep review (verify) — 2 resolved · 0 open
| # | Severity | Area | Finding | Location | Outcome |
|---|---|---|---|---|---|
| 1 | 🟡 Minor | Correctness | A CSS length on Theme columnGap was answered with a gap step instead of the bulmaVars remedy |
eslint-plugin/src/rules/valid-helper-value.ts:66 |
✅ Verified fixed |
| 2 | 🟡 Minor | Correctness | On Columns, the gap report advertised half steps that render nothing there |
eslint-plugin/src/lib/values.ts:286 |
✅ Verified fixed |
What I checked
- #1 —
suggest()now returns an empty list for a number-with-unit (/^[\d.]+[a-z%]+$/i,valid-helper-value.ts:68), so<Theme columnGap="8px">and"1em"fall through todeprecatedVariable, the branch that prints thebulmaVarsremedy naming--bulma-column-gap. The suite pins that message bymessageIdplus data, and still pins the unitless near misses (rowGap="1.25"suggests1.5/0.5/2.5;aspectRatio="4by1"staysinvalidWithSuggestion), so the guard did not over-reach into the cases it was meant to leave alone. - #2 —
ELEMENT_VALUESis keyed by element and read ahead ofHELPER_VALUES(valid-helper-value.ts:144), with thegapofColumnsderived asvalidGaps.filter(step => !step.includes('.'))rather than copied. Tests pin the full rendered message for<Columns gap="1rem" />— whole steps only — and report<Columns gap={1.5} />throughnumericStepInvalidagainst that same narrowed list, whilegap="2"andgap={4}stay valid. pnpm exec turbo run test --filter=@allxsmith/eslint-plugin-bestax— 7 suites, 256 tests, green. It buildsbulma-uifirst, so the tuples come from the realdist.
Overall: This pass settled the two threads the earlier review left open and reviewed no commits. Both fixes are in the current code, both are pinned by tests that assert the rendered message rather than only a messageId, and the plugin suite is green. Nothing remains open from that review; a reader who wants the commits pushed since judged on their own should ask for a fresh review.
Residual risk: Not re-assessed in a verify pass — scoped to the two threads above, both confirmed against current code and the suite. The one trade-off worth noting from the first fix, which was my own suggestion applied as written, is that the length guard also suppresses suggestions for a non-length value shaped digits-then-letters; the rule still reports such a value with the full valid list, so the advice stays correct, just less pointed.
🏄 Paddled back out to check the two waves I called last set, and both got ridden clean — the
8pxdrop-in now sends you tobulmaVarsinstead of a bogus 4rem step, andColumnsfinally quotes only the gutters it actually surfs. Threads closed, board waxed, good to paddle on.
allxsmith
added
the
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 |
|---|---|
 |
 |
allxsmith
added
the
review-converged
|
🎉 This PR is included in version 5.26.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.25.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 4.2.13 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.14.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
1 participant
Part of #769, the second of three PRs after #877. It brings the ESLint plugin up to the gap helper props, and makes Theme's
columnGapthe helper, which has to land with it:scripts/helper-prop-collisions.test.mjsholds the variables Theme writes by name to the plugin's declared routes.Please merge with a merge commit, not a squash. The commits carry two release scopes (bulma-ui, eslint-plugin).
One behaviour change, on
Theme, for you to confirm. Theme turns every Bulma variable into a camelCase prop, so--bulma-column-gaphas been an untypedcolumnGapprop.columnGapbecomes the column gap helper on Theme, the wayradiusandshadowalready are."1rem", still sets the variable, with a development warning pointing atbulmaVars. That's how [Bug] bulma-ui: Theme'sradiusprop is typed as the helper but behaves as the CSS variable #694 kept the oldradiusroute.<Theme columnGap="0">used to zero the columns gutter inside the Theme, and now addsis-column-gap-0to the wrapper.isRootTheme it's sharper:<Theme isRoot columnGap="0">used to zero the gutters page-wide, and a gap step there now does nothing and warns in development, like the radius sizes. The remedy for either isbulmaVars={{ '--bulma-column-gap': '0' }}, which the warning names.ESLint plugin:
valid-helper-valuechecksgap,columnGapandrowGapagainstvalidGaps, and accepts the steps as numbers too, so<Grid gap={3} />and<Box gap={1.5} />pass. A number that isn't a step gets its own message.columnGapjoinsradiusin the deprecated variable routes, so the plugin points a variable-style value atbulmaVars.pnpm allpasses locally.