Repository navigation
Conversation
Adds a lightweight IntersectionObserver-backed wrapper that animates content into view as it scrolls into the viewport (fade, fade-up, fade-down, slide-left, slide-right, zoom, flip), with a cascade mode that staggers direct children. Accessibility and progressive enhancement are built in: it skips the animation under prefers-reduced-motion, renders the final visible state during SSR and on first client render, and falls back to visible immediately when IntersectionObserver is unavailable. A custom (non-forwardRef) component passed via `as` falls back to an internal wrapper div so scroll observation still attaches a ref. 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 due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a new ChangesReveal Component
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant Reveal
participant IntersectionObserver
participant DOM
Browser->>Reveal: mount component
Reveal->>Reveal: detect reduced motion
alt motion reduced
Reveal->>DOM: render visible state
else motion allowed
Reveal->>IntersectionObserver: observe with threshold
IntersectionObserver-->>Reveal: enter viewport
Reveal->>DOM: apply revealed classes
end
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Preview DeploymentPreview URL: https://da6bb4af.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 — 1 finding
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🟡 Minor | API | Shipped skills-catalog entry truncated mid-sentence ("...viewport, backed"); the Overview's first line wraps mid-sentence so the generator emits a dangling half-sentence | docs/docs/api/components/reveal.md:10 |
Overall: This is a clean, well-engineered addition. The Reveal component gets the hard parts right — SSR renders the final visible state so first-paint markup matches the server (no hydration mismatch), prefers-reduced-motion and missing-IntersectionObserver both fall back to the revealed state, threshold is clamped/guarded against the RangeError that IO throws for NaN/out-of-range, and the non-forwardRef as={Component} case is handled by wrapping in an observed div and documented in three places. Tests are thorough (26 passing, 100% stmt/branch/func/line on the new file), and the story, API docs page, and regenerated catalog are all present. The only real defect is the truncated catalog description, which ships to LLM consumers via create-bestax. Worth a human eye (non-blocking): the as={Component} case puts className/helper classes on the wrapper div rather than the component — necessary and documented, but a behavioral divergence from other as-supporting components; and above-the-fold content will briefly flash visible→hidden→animate-in on hydration, an inherent tradeoff of the SSR-safe approach.
🏄 Totally smooth ride, dude — this Reveal catches the scroll wave clean, bails gracefully when the user wants no motion, and doesn't wipe out on SSR. Just one gnarly little sentence that got clipped before it hit the beach; patch that and it's good to paddle out.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
bulma-ui/src/scss/components/_reveal.scss (1)
31-43: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
will-changepersists indefinitely after reveal.
will-change: opacity, transformis applied to the base class (Line 42) and never removed once.is-revealedis added (Lines 70-79). Per MDN guidance,will-change"implies that the targeted elements are always a few moments away from changing" and browsers "keep the optimizations for much longer time than it would have otherwise" when set directly in a stylesheet rather than toggled via script. Given this component targets landing pages with many staggered/cascaded elements, leavingwill-changeon indefinitely can force the browser to maintain composited layers for all revealed elements, increasing memory usage.♻️ Proposed fix: drop the will-change hint 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: 70-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 base reveal selector block in _reveal.scss applies will-change: opacity, transform permanently, so update the reveal styles to stop hinting after the element becomes visible. Adjust the .#{iv.$class-prefix}reveal-* rules and the .is-revealed state so will-change is only present while an element is animating, and is removed or reset once .is-revealed is applied. Use the existing reveal class names and the .is-revealed selector to keep the fix scoped to the reveal component.bulma-ui/src/components/Reveal.stories.tsx (1)
27-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor duplication of animation option list.
The
animationlist is duplicated betweenargTypes.options(Lines 27-35) and the localANIMATIONSarray (Lines 92-100). Consider deriving one from the other (or fromRevealAnimation) to avoid drift if new variants are added.Also applies to: 92-100
🤖 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.stories.tsx` around lines 27 - 35, The animation option list is duplicated between the Storybook argTypes and the local animation constant, which can drift over time. Update Reveal.stories.tsx so the argTypes options are derived from the existing ANIMATIONS source, or from RevealAnimation directly, and keep ANIMATIONS as the single source of truth used by the Reveal story.bulma-ui/src/components/Reveal.tsx (1)
124-125: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMixed usage of
prefixedClassNamesandusePrefixedClassNames.Line 179 manually calls
prefixedClassNames(classPrefix, {...})(requiring the separateuseClassPrefix()call at Line 125), while Line 185 uses theusePrefixedClassNameshook directly. Consolidating on one helper (likelyusePrefixedClassNames, matching the hook-based pattern already used at Line 185) would remove the need for the extraclassPrefixvariable and keep prefixing logic consistent within the component.Also applies to: 133-135, 179-189
🤖 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` around lines 124 - 125, The Reveal component mixes direct prefixedClassNames usage with the usePrefixedClassNames hook, creating inconsistent prefixing and an extra useClassPrefix dependency. Update Reveal to use one approach consistently, preferably usePrefixedClassNames alongside the existing hook-based pattern, and remove the separate classPrefix handling from the component. Adjust the affected class-building logic in the Reveal component so all prefixing flows through the same helper.
🤖 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 228-253: `Reveal` currently wraps non-string `as` components in a
plain `<div>`, which breaks Bulma layout primitives like `Column`, `Cell`, and
`cascade` because the wrapper becomes the direct child of the container. Update
`Reveal` to either require `as` components that forward refs so the animation
node can be attached directly, or explicitly disallow/document layout-sensitive
Bulma primitives in the `Component`/`as` path; focus on the `Reveal` render
branch that distinguishes `typeof Component === 'string'` from custom
components.
In `@skills/bestax-custom-component/references/component-catalog.md`:
- Line 73: The Reveal catalog entry is being truncated mid-sentence because the
source overview in reveal.md is wrapped across multiple lines and the generator
is only capturing the first line. Fix the source description or update the
component catalog generation logic in the relevant overview parsing/generation
path so the full sentence is preserved, then regenerate the catalog with pnpm
gen:catalog instead of editing component-catalog.md directly.
---
Nitpick comments:
In `@bulma-ui/src/components/Reveal.stories.tsx`:
- Around line 27-35: The animation option list is duplicated between the
Storybook argTypes and the local animation constant, which can drift over time.
Update Reveal.stories.tsx so the argTypes options are derived from the existing
ANIMATIONS source, or from RevealAnimation directly, and keep ANIMATIONS as the
single source of truth used by the Reveal story.
In `@bulma-ui/src/components/Reveal.tsx`:
- Around line 124-125: The Reveal component mixes direct prefixedClassNames
usage with the usePrefixedClassNames hook, creating inconsistent prefixing and
an extra useClassPrefix dependency. Update Reveal to use one approach
consistently, preferably usePrefixedClassNames alongside the existing hook-based
pattern, and remove the separate classPrefix handling from the component. Adjust
the affected class-building logic in the Reveal component so all prefixing flows
through the same helper.
In `@bulma-ui/src/scss/components/_reveal.scss`:
- Around line 31-43: The base reveal selector block in _reveal.scss applies
will-change: opacity, transform permanently, so update the reveal styles to stop
hinting after the element becomes visible. Adjust the
.#{iv.$class-prefix}reveal-* rules and the .is-revealed state so will-change is
only present while an element is animating, and is removed or reset once
.is-revealed is applied. Use the existing reveal class names and the
.is-revealed selector to keep the fix scoped to the reveal component.
🪄 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: ab71c67f-fb79-4acb-a2fe-cc0709c34ca5
📒 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 — |
…t truncated The Reveal overview's first sentence wrapped across two physical lines (breaking at "backed"), and gen-component-catalog.mjs reads only the first physical line after the Overview heading, so the shipped skills catalog entry was clipped mid-sentence. Keep the full first sentence on one line and regenerate the catalog. Also note in the as-component admonition that Bulma layout primitives (Column/Cell) shouldn't be passed as as, since the observed wrapper div breaks their required direct-child relationship. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Preview DeploymentPreview URL: https://99600ee1.bestax.pages.dev |
AI fix pass — iteration 1 recapAll three open review threads handled; one commit pushed ( Fixed
Refuted (in part)
Gates run
|
|
🏄 Surf's up and I rode the whole set — total convergence, brah. 1 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
|
Closing this one — re-running the loop fresh from #197 to exercise the newly merged workflow changes. A new PR will supersede it. Generated by Claude Code |
2 participants
Summary
Implements the
Revealcomponent proposed in #197: a lightweight wrapper backed byIntersectionObserverthat animates content into view as it scrolls into the viewport —fade,fade-up,fade-down,slide-left,slide-right,zoom,flip— plus acascademode that staggers direct children with an incrementing delay.Accessibility/progressive enhancement is built in, not opt-in:
prefers-reduced-motion: reduceis set.IntersectionObserverisn't available.forwardRef) components passed viaas(e.g.Section,Card) by falling back to an internal wrapperdivfor scroll observation, so the ref always attaches — with a regression test covering it.This is the fourth attempt at this issue: PR #243 and PR #247 each fully implemented and passed all gates but were closed without merging (no reason recorded); a third attempt was blocked purely on tooling permissions and never opened a PR. This PR carries forward the same finalized implementation, rebuilt on a fresh branch off current
mainand re-verified from scratch.Changes
bulma-ui/src/components/Reveal.tsx— the componentbulma-ui/src/components/Reveal.stories.tsx— Storybook stories (Default, Animations, AsSection, Cascade, ScrollToReveal)bulma-ui/src/components/__tests__/Reveal.test.tsx— 22 tests, 100% coverage onReveal.tsxbulma-ui/src/scss/components/_reveal.scss(+ registered in_index.scss) — CSS-variable-driven animation styles, including aprefers-reduced-motionfallbackbulma-ui/src/index.ts— public exportdocs/docs/api/components/reveal.md— API docs pageskills/bestax-custom-component/references/component-catalog.md— regenerated (pnpm gen:catalog)Test plan
pnpm --filter @allxsmith/bestax-bulma run typecheck— cleanpnpm --filter @allxsmith/bestax-bulma run lint— 0 errors (only pre-existing unrelated warnings)pnpm --filter @allxsmith/bestax-bulma run test:coverage— 3353 tests pass,Reveal.tsx100% coverage, overall thresholds held (99.37% stmts / 99.05% branches)pnpm run format:check— cleanpnpm run gen:catalog:check— catalog matchesFixes #197
Generated with Claude Code
Summary by CodeRabbit
Revealcomponent with configurable animation, timing, and thresholds.asrendering support.Reveal, including reduced-motion, SSR, and fallback behavior.Reveal.once/replay, cascade timing, and reduced-motion updates.