Repository navigation
Conversation
Adds a Reveal wrapper component backed by IntersectionObserver that animates content into view as it scrolls into the viewport (fade, fade-up/down, slide-left/right, zoom, flip), with optional staggered cascade of direct children. Automatically honors prefers-reduced-motion and renders the final, visible state during SSR and on the first client render, so content is never hidden if JavaScript never runs. Fixes #197 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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a new ChangesReveal Component Feature
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant Reveal
participant IntersectionObserver
Browser->>Reveal: mount component
Reveal->>Reveal: check prefers-reduced-motion
Reveal->>IntersectionObserver: observe element
IntersectionObserver->>Reveal: intersection event
Reveal->>Reveal: set isRevealed, compute classes/styles
Reveal->>Browser: render revealed markup
Related Issues: Suggested labels: enhancement, component, documentation Suggested reviewers: allxsmith 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Preview DeploymentPreview URL: https://5d8fd9d8.bestax.pages.dev |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| return ( | ||
| <div ref={setNode} className={combinedClasses} style={wrapperStyle}> | ||
| <Component {...rest}>{content}</Component> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
The as-with-a-component path silently changes DOM structure, which diverges from every other polymorphic as in the library and can break layout contracts.
For an intrinsic string (as="section") the reveal wrapper becomes that element. But for a React component (as={Section}, as={Column}, …) the component is instead nested inside an extra reveal div, producing <div class="reveal reveal-fade-up"><section>…</section></div> rather than <section class="reveal …">.
This is inconsistent with the rest of the library, where as/polymorphic components (Button, Navbar.Item, Menu.Item) render as the given component with no wrapper. Concretely it can break CSS that relies on a direct parent→child relationship — e.g. <Columns><Reveal as={Column} …>…</Reveal></Columns> puts a non-.column div as the flex child of .columns, collapsing the column layout. The AsSection story and the docs ("Rendered as a Section … instead of the default div") also read as if as replaces the wrapper, which isn't what happens for components.
The ref-forwarding constraint that motivates the fallback is understood and documented in the code comment — flagging so a human can confirm the extra-wrapper behavior (and the docs/story wording) is the intended trade-off rather than a surprise for consumers who reach for as with a layout component.
There was a problem hiding this comment.
Refuting as a fix-pass change and leaving this open for the maintainer to rule on, which is what the flag asks for.
The divergence is real and matches the issue 197 wording (as renders the wrapped element). But honoring it for a React component while still observing scroll is not a small fix: IntersectionObserver needs a real DOM node, and every candidate target in this library (Section, Card, Column, and so on) is a plain React.FC with no ref forwarding, so a ref cannot attach to them. The only ways to make as set to Column render as a real .column and still be observed are (a) add forwardRef plus a merged ref to every target component across the library, or (b) redesign Reveal to forward a ref into the child element. Both are cross-cutting public-API changes well beyond the scope of issue 197 (add a Reveal component), and the fix-pass guidance here is explicitly to not undertake large architecture refactors to satisfy a single review.
So this is a deliberate, code-documented trade-off: see the comment above the branch stating that a custom component passed via as is rendered with no ref forwarding, so the ref goes on a plain wrapper div. For layout-sensitive nesting such as a Reveal set to Column placed directly inside Columns, the intended usage is an intrinsic tag or a normal wrapper. Deferring to the maintainer: if you want true render-as-component semantics that is a follow-up API change; if the wrapper fallback is acceptable, the docs and story wording should be tightened to say the element is wrapped, not replaced. Not resolving, so this escalates for a human decision.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
bulma-ui/src/components/Reveal.tsx (1)
36-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the internal wrapper-div behavior for non-intrinsic
as.When
asis a custom component (non-string), Reveal always wraps it in an internaldivcarrying the animation/Bulma classes, while the renderedComponentonly getsrest(noclassName/bulmaHelperClasses). The code comment at lines 222-227 explains this well internally, but the public JSDoc foras(line 36) doesn't mention the extra wrapper or the class-placement difference, which could surprise consumers targeting the rendered element for styling.Also applies to: 222-247
🤖 Prompt for 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. In `@bulma-ui/src/components/Reveal.tsx` at line 36, The JSDoc for Reveal’s as prop is incomplete for custom components: it should document that non-intrinsic (non-string) as values are wrapped in an internal div that receives the animation/Bulma classes, while the custom Component only receives rest props. Update the public comment near as and, if needed, the Render logic in Reveal to clearly reflect the wrapper-div behavior and className placement so consumers know where styling applies.bulma-ui/src/scss/components/_reveal.scss (1)
31-43: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider resetting
will-changeafter reveal.
will-change: opacity, transformis applied to all animated variants but never reset (e.g. toauto) once.is-revealedis added. Forcascadegrids with many children this keeps many elements promoted to their own compositing layer indefinitely, which can add memory overhead beyond the point where it's useful.♻️ Optional: clear will-change once revealed
.#{iv.$class-prefix}reveal-fade.#{iv.$class-prefix}is-revealed, .#{iv.$class-prefix}reveal-fade-up.#{iv.$class-prefix}is-revealed, .#{iv.$class-prefix}reveal-fade-down.#{iv.$class-prefix}is-revealed, .#{iv.$class-prefix}reveal-slide-left.#{iv.$class-prefix}is-revealed, .#{iv.$class-prefix}reveal-slide-right.#{iv.$class-prefix}is-revealed, .#{iv.$class-prefix}reveal-zoom.#{iv.$class-prefix}is-revealed, .#{iv.$class-prefix}reveal-flip.#{iv.$class-prefix}is-revealed { opacity: 1; transform: none; + will-change: auto; }Also applies to: 69-79
🤖 Prompt for 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. In `@bulma-ui/src/scss/components/_reveal.scss` around lines 31 - 43, The reveal animation styles in the `_reveal.scss` ruleset keep `will-change: opacity, transform` set on all variants even after the element becomes `.is-revealed`, which can leave many promoted layers around unnecessarily. Update the reveal styles for the shared selectors and the `.is-revealed` state so `will-change` is cleared or reset once the animation is done, and apply the same change to the matching cascade block referenced in the comment. Use the existing reveal variant selectors and `.is-revealed` rule to keep the fix localized.
🤖 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/components/Reveal.tsx`:
- Around line 110-125: The Reveal component has a potential ref collision issue
when the `as` prop is used with a custom component. The `setNode` ref callback
is being set on the rendered component (whether it's the intrinsic `Component`
or a custom element), but the `...rest` spread operator can override it if a
`ref` prop is passed through props. Extract the `ref` from the destructured
props before spreading, then merge it appropriately with the `setNode` callback
so that both the observer's ref and any user-provided ref are properly handled
when rendering the component specified by the `as` parameter.
In `@bulma-ui/src/scss/components/_reveal.scss`:
- Around line 45-59: The negative reveal transforms in the _reveal.scss rules
use string interpolation that produces invalid CSS, so update the
reveal-fade-down and reveal-slide-right selectors to negate the cv.getVar(...)
values with calc() instead of -#{...}. Keep the existing transform declarations
in the .#{iv.$class-prefix}reveal-fade-down and
.#{iv.$class-prefix}reveal-slide-right blocks, but change the offset negation
approach so the generated CSS remains valid.
In `@docs/docs/api/components/reveal.md`:
- Around line 1-4: The Reveal docs frontmatter is missing the required docs
ordering field, so update the frontmatter in the Reveal markdown page to include
sidebar_position alongside title and sidebar_label. Make sure the new field is
placed in the existing frontmatter block for this page so the Docusaurus docs
sidebar can order it correctly.
---
Nitpick comments:
In `@bulma-ui/src/components/Reveal.tsx`:
- Line 36: The JSDoc for Reveal’s as prop is incomplete for custom components:
it should document that non-intrinsic (non-string) as values are wrapped in an
internal div that receives the animation/Bulma classes, while the custom
Component only receives rest props. Update the public comment near as and, if
needed, the Render logic in Reveal to clearly reflect the wrapper-div behavior
and className placement so consumers know where styling applies.
In `@bulma-ui/src/scss/components/_reveal.scss`:
- Around line 31-43: The reveal animation styles in the `_reveal.scss` ruleset
keep `will-change: opacity, transform` set on all variants even after the
element becomes `.is-revealed`, which can leave many promoted layers around
unnecessarily. Update the reveal styles for the shared selectors and the
`.is-revealed` state so `will-change` is cleared or reset once the animation is
done, and apply the same change to the matching cascade block referenced in the
comment. Use the existing reveal variant selectors and `.is-revealed` rule to
keep the fix localized.
🪄 Autofix (Beta)
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: 0e807948-062c-4a7b-99f9-8f04964eae3c
📒 Files selected for processing (8)
bulma-ui/src/components/Reveal.stories.tsxbulma-ui/src/components/Reveal.tsxbulma-ui/src/components/__tests__/Reveal.test.tsxbulma-ui/src/index.tsbulma-ui/src/scss/components/_index.scssbulma-ui/src/scss/components/_reveal.scssdocs/docs/api/components/reveal.mdskills/bestax-custom-component/references/component-catalog.md
|
AI loop status: iteration 1/4 — |
…ide-right animate Negating an interpolated custom property produced invalid CSS and dropped the reveal-fade-down and reveal-slide-right transforms. Matches the _slider.scss calc idiom. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nsic path In the string-as branch, ...rest was spread after ref: setNode, so a ref reaching rest (React 19 passes ref as a regular prop to function components) could clobber the IntersectionObserver callback and silently break scroll detection. Spread ...rest first so the internal ref/className/style always win; className and style are already destructured out of rest, so only the observer ref is affected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Preview DeploymentPreview URL: https://9a178f4b.bestax.pages.dev |
Preview DeploymentPreview URL: https://9abfe932.bestax.pages.dev |
AI fix pass (iteration 1 of 4)Two concrete, in-scope defects fixed; two findings refuted or deferred. The working tree was read-only this run, so I committed the fixes via the GitHub API and could not run local gates. CI runs the full pnpm all gate on the pushed commits. Fixed
Refuted / deferred
GatesCould not be run locally (read-only sandbox this run); relying on CI for lint, typecheck, test:coverage, format:check, and gen:catalog:check. Both changes are low-risk: the SCSS matches a proven idiom already used in _slider.scss, and the Reveal.tsx change is a pure property reorder (zero length delta) that cannot alter types or existing test behavior. gen:catalog is unaffected since no props or exports changed. |
On PR #243 the fixer did the right work but the human-readable explanation lagged: commits landed ~04:50-04:55, but the plain-English thread replies and the summary comment did not post until 05:06-05:15. For ~15-20 min the PR showed commits with no explanation, so it read as 'just committing, not engaging' even though the write-up was thorough once it arrived. Restructure the fix prompt so the agent engages like a human reviewer, as it goes rather than in one end-of-pass batch: - Handle threads one at a time and reply in each thread the moment it is addressed (fix + commit + reply, or refute + reply) — no deferring replies to the end. - Write replies in plain English, first person, saying what changed and why (or why it disagrees) with concrete code citations — not bare markers. - The end-of-pass summary is now an explicit RECAP of the per-thread replies, not the first place the reasoning appears. State-machine markers are preserved: claude-review fix replies still START with the literal 'Fixed in <full-sha>' token (the verify step keys off it), CodeRabbit/claude threads are still never resolved by the fixer, Copilot threads are still resolved via the GraphQL mutation. Claude-Session: https://claude.ai/code/session_01NVR5yWevceZEFbTJmpviiM Co-authored-by: Claude <noreply@anthropic.com>
…and 2x turns (#245) The deep review posted terse, jumbled-paragraph findings and a one-line summary — hard for a human to scan and thinner than CodeRabbit's structured output. Rework the reviewer prompt so its output is both AI-actionable and human-readable: - Every inline finding is now structured Markdown: bold title + severity (Critical/Major/Minor) + category tag, What / Why it matters / Fix (a suggestion block or fenced diff), long traces folded into <details>, and a mermaid diagram when a flow picture beats prose. - The certifying summary review is now a severity table + an Overall read + a short, human 'California surfer' blurb — written to a file and posted via gh api -F body=@file so the tables/markdown survive intact. The machine-parsed <!-- claude-deep-review --> marker stays the first line. - Double the reviewer's budget (--max-turns 60 -> 120): richer, verified, well-formatted findings need more room, and the deep review missed two real bugs on #243 that CodeRabbit caught with deeper per-file analysis. Follow-up (separate PR): the bounded 3-round reviewer<->bestaxbot rebuttal on refuted findings, which needs gate/state-machine changes. Claude-Session: https://claude.ai/code/session_01NVR5yWevceZEFbTJmpviiM Co-authored-by: Claude <noreply@anthropic.com>
… MCP (#250) Implement run #18 fully built the Reveal component (gates green, 100% coverage) but produced no branch/PR: it reached for mcp__github_file_ops__commit_files, which is not allowlisted, and gave up blocked — even though the git-CLI commit/push verbs ARE allowlisted and are what runs #16/#17 used to open #243 and #247. Non-deterministic tool choice, same class as the fixer reaching for a nonexistent MCP reply tool. - Rewrite step 4 with an explicit git-CLI commit+push recipe (git checkout -b / add / commit / push) and state plainly that there is NO MCP commit tool here — git is already set up to SSH-sign as bestaxbot, so git-CLI commits come out Verified. - Add show_full_output so a blocked commit is visible in the log, not just an artifact the proxy won't let us download. Claude-Session: https://claude.ai/code/session_01NVR5yWevceZEFbTJmpviiM Co-authored-by: Claude <noreply@anthropic.com>
2 participants
Summary
Adds a
Revealcomponent: a lightweight, CSS-driven wrapper backed byIntersectionObserverthat animates content into view as it scrolls into theviewport (
fade,fade-up,fade-down,slide-left,slide-right,zoom,flip), with an optionalcascademode that staggers direct children with anincrementing delay.
Accessibility/progressive-enhancement are built in, not opt-in:
the user prefers reduced motion (
prefers-reduced-motion: reduce).so content is never hidden if JavaScript never runs (crawlers, no-JS).
IntersectionObserverisn't available.
While implementing this, I found and fixed a real bug: passing a plain
(non-
forwardRef) component asas— which is how every component in thislibrary, including
Section/Card, is written — silently broke scrolldetection, since the ref never attached and content stayed hidden forever.
Fixed by falling back to an internal wrapper
divfor scroll observationwhenever
asisn't a plain intrinsic HTML tag, with a regression testcovering it (
Reveal.test.tsx: "still observes and reveals when `as` is anon-forwardRef component").
Fixes #197
Changes
bulma-ui/src/components/Reveal.tsx— the componentbulma-ui/src/components/Reveal.stories.tsx— Storybook stories (includinga scroll-to-reveal demo)
bulma-ui/src/components/__tests__/Reveal.test.tsx— 22 tests, 100% coveragebulma-ui/src/scss/components/_reveal.scss— animation styles, registeredin
_index.scssdocs/docs/api/components/reveal.md— API docs pagebulma-ui/src/index.ts— public exportskills/bestax-custom-component/references/component-catalog.md—regenerated via
pnpm gen:catalogTest plan
pnpm --filter @allxsmith/bestax-bulma exec tsc --noEmitpnpm --filter @allxsmith/bestax-bulma run lintpnpm --filter @allxsmith/bestax-bulma run test:coverage(100% onReveal.tsx, no threshold regressions; 3350 tests passing)pnpm run format:checkpnpm run gen:catalog:checkGenerated with Claude Code
Summary by CodeRabbit
New Features
Revealcomponent with multiple animation styles, viewport-triggered visibility, reduced-motion support, staggered child reveals (cascade), and polymorphic rendering viaas.Revealin the public package API and included matching styling.Documentation
Reveal.Reveal.Tests