feat(accordion): add transitionDuration and scrollOnOpen props #5828
Conversation
🦋 Changeset detectedLatest commit: 057eda1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@KMNowak is attempting to deploy a commit to the HeroUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdds two optional Accordion props — Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Accordion
participant AccordionItem
participant ContentRef as contentRef
User->>Accordion: click header
Accordion->>AccordionItem: toggle isOpen
AccordionItem->>AccordionItem: run animation (duration = transitionDuration || 300ms)
alt scrollOnOpen = true
AccordionItem->>AccordionItem: useLayoutEffect on isOpen && scrollOnOpen
Note right of AccordionItem #e6f7ff: double requestAnimationFrame to wait for layout
AccordionItem->>ContentRef: contentRef.scrollIntoView({ behavior: "smooth", block: "nearest" })
end
AccordionItem-->>User: content rendered/expanded
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
.changeset/shy-kiwis-bake.md (1)
5-6: Minor: Simplify the phrasing.The phrase "as it is right now" is slightly informal for a changelog. Consider simplifying to maintain consistency with release note conventions.
Apply this diff:
-- Introduce new prop `transitionDuration?: number` to customize animation speed. Defaults to 300ms as it is right now +- Introduce new prop `transitionDuration?: number` to customize animation speed. Defaults to 300mspackages/components/accordion/stories/accordion.stories.tsx (2)
379-401: Good showcase for scrollOnOpenTemplate correctly uses a scrollable container to demonstrate behavior. Consider adding Storybook controls so consumers can toggle scrollOnOpen on any story.
Apply this (outside this hunk) to expose controls:
argTypes: { + scrollOnOpen: { + control: {type: "boolean"}, + }, + transitionDuration: { + control: {type: "number", min: 0, max: 3000, step: 50}, + table: {type: {summary: "number (ms)"}, defaultValue: {summary: 300}}, + }, },Based on learnings.
402-429: Nice coverage of fast/slow durationsClear contrast between 150ms and 800ms. Consider adding one example wired to args.transitionDuration so users can live-edit the value via controls (pairs well with the argTypes suggestion above).
packages/components/accordion/src/accordion-item.tsx (1)
82-93: Preserve existing easing/type when applying custom durationReplacing the entire transition may drop easing/type from TRANSITION_VARIANTS. Merge and set per‑property durations to keep behavior consistent.
- const transitionVariants: Variants = { - exit: { - ...TRANSITION_VARIANTS.collapse.exit, - overflowY: "hidden", - transition: {duration: transitionDuration ? transitionDuration / 1000 : 0.3}, - }, - enter: { - ...TRANSITION_VARIANTS.collapse.enter, - overflowY: "unset", - transition: {duration: transitionDuration ? transitionDuration / 1000 : 0.3}, - }, - }; + const dur = (transitionDuration ?? 300) / 1000; + const transitionVariants: Variants = { + exit: { + ...TRANSITION_VARIANTS.collapse.exit, + overflowY: "hidden", + transition: { + ...(TRANSITION_VARIANTS.collapse.exit as any).transition, + height: {...(TRANSITION_VARIANTS.collapse.exit as any)?.transition?.height, duration: dur}, + opacity: {...(TRANSITION_VARIANTS.collapse.exit as any)?.transition?.opacity, duration: dur}, + }, + }, + enter: { + ...TRANSITION_VARIANTS.collapse.enter, + overflowY: "visible", + transition: { + ...(TRANSITION_VARIANTS.collapse.enter as any).transition, + height: {...(TRANSITION_VARIANTS.collapse.enter as any)?.transition?.height, duration: dur}, + opacity: {...(TRANSITION_VARIANTS.collapse.enter as any)?.transition?.opacity, duration: dur}, + }, + }, + };If you prefer to keep types strict, mirror the structure of TRANSITION_VARIANTS locally instead of casting.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.changeset/shy-kiwis-bake.md(1 hunks)apps/docs/content/components/accordion/index.ts(2 hunks)apps/docs/content/components/accordion/scroll-on-open.raw.jsx(1 hunks)apps/docs/content/components/accordion/scroll-on-open.ts(1 hunks)apps/docs/content/components/accordion/transition-duration.raw.jsx(1 hunks)apps/docs/content/components/accordion/transition-duration.ts(1 hunks)apps/docs/content/docs/components/accordion.mdx(2 hunks)packages/components/accordion/__tests__/accordion.test.tsx(1 hunks)packages/components/accordion/src/accordion-item.tsx(6 hunks)packages/components/accordion/src/use-accordion-item.ts(3 hunks)packages/components/accordion/src/use-accordion.ts(6 hunks)packages/components/accordion/stories/accordion.stories.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
apps/docs/content/components/accordion/scroll-on-open.ts (1)
apps/docs/content/components/accordion/scroll-on-open.raw.jsx (1)
App(3-65)
apps/docs/content/components/accordion/transition-duration.ts (1)
apps/docs/content/components/accordion/transition-duration.raw.jsx (1)
App(3-67)
packages/components/accordion/stories/accordion.stories.tsx (2)
packages/components/accordion/src/accordion.tsx (1)
AccordionProps(9-9)packages/components/accordion/src/index.ts (3)
AccordionProps(5-5)Accordion(14-14)AccordionItem(14-14)
apps/docs/content/components/accordion/scroll-on-open.raw.jsx (1)
apps/docs/content/components/accordion/transition-duration.raw.jsx (2)
App(3-67)defaultContent(4-5)
apps/docs/content/components/accordion/transition-duration.raw.jsx (1)
apps/docs/content/components/accordion/scroll-on-open.raw.jsx (2)
App(3-65)defaultContent(4-5)
🪛 LanguageTool
.changeset/shy-kiwis-bake.md
[grammar] ~6-~6: Ensure spelling is correct
Context: ... customize animation speed. Defaults to 300ms as it is right now
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (13)
packages/components/accordion/src/use-accordion-item.ts (1)
38-47: LGTM! Clean prop additions with sensible defaults.The new props are well-documented, use reasonable defaults (300ms matches common animation standards, scrollOnOpen disabled by default to avoid surprising behavior), and are properly threaded through the hook's return value.
Also applies to: 77-78, 284-285
packages/components/accordion/src/use-accordion.ts (1)
48-57: LGTM! Consistent parent hook implementation.The props are properly threaded through the accordion parent hook, included in the values context payload, dependency arrays, and return object. This ensures child items receive the configuration correctly.
Also applies to: 84-85, 118-119, 212-213, 228-229, 265-266
apps/docs/content/components/accordion/index.ts (1)
17-18: LGTM! Proper documentation integration.The new demo imports and exports follow the existing pattern for accordion documentation examples.
Also applies to: 37-38
apps/docs/content/components/accordion/scroll-on-open.ts (1)
1-9: LGTM! Standard documentation demo module.This follows the established pattern for exposing demo components to the documentation system.
apps/docs/content/components/accordion/transition-duration.ts (1)
1-9: LGTM! Standard documentation demo module.This follows the established pattern for exposing demo components to the documentation system.
packages/components/accordion/__tests__/accordion.test.tsx (2)
382-410: LGTM! Reasonable timing verification for custom duration.The test validates that the custom transition duration is respected by checking opacity values during and after the animation. The
customDuration + 10msbuffer is reasonable for allowing Framer Motion to complete its animation cycle.
350-380: Remove the timing concern—100ms wait is sufficient for the implementation.The actual scrollIntoView implementation in
accordion-item.tsx(line 52-57) uses a 50mssetTimeoutbefore callingscrollIntoView. The test waits 100ms before asserting, which provides a 50ms buffer. The mocked scrollIntoView call completes synchronously (it's a mock, not the actual browser implementation), so the 100ms wait is adequate and not prone to flakiness in CI environments.Likely an incorrect or invalid review comment.
apps/docs/content/docs/components/accordion.mdx (1)
125-135: LGTM! Comprehensive documentation for new features.The documentation clearly explains both new props with live demos and accurate API table entries. The placement in the docs flow is logical, and the default values match the implementation.
Also applies to: 296-307
apps/docs/content/components/accordion/transition-duration.raw.jsx (1)
7-66: Demo looks goodAccurately showcases the new transitionDuration prop with sensible examples. No issues found.
packages/components/accordion/stories/accordion.stories.tsx (2)
19-19: Import reorder is fineReordered named imports to include AccordionItem/Props alongside Accordion/Props. No behavior change.
583-597: Exports are wired correctlyStories render the new templates with default props. LGTM.
apps/docs/content/components/accordion/scroll-on-open.raw.jsx (1)
7-64: Clear side‑by‑side demoAccurately demonstrates behavior with and without scrollOnOpen in realistic scroll containers. No issues.
packages/components/accordion/src/accordion-item.tsx (1)
109-112: Ref usage is correctAttaching contentRef in both animated and non‑animated branches ensures the scroll target is present. LGTM.
Please confirm that use-accordion-item forwards scrollOnOpen/transitionDuration from Accordion to AccordionItem in all selection modes.
Also applies to: 130-133, 75-79
KMNowak
force-pushed
the
feat/accordion_transitionDuration_and_scrollOnOpen
branch
from
a2fdaa9 to
cd857c6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.changeset/shy-kiwis-bake.md (1)
6-6: Simplify the wording for clarity.The phrase "as it is right now" is informal and slightly awkward. Consider simplifying to just "Defaults to 300ms" since this is establishing the default behavior going forward.
Apply this diff:
-- Introduce new prop `transitionDuration?: number` to customize animation speed. Defaults to 300ms as it is right now +- Introduce new prop `transitionDuration?: number` to customize animation speed. Defaults to 300msapps/docs/content/components/accordion/transition-duration.raw.jsx (1)
1-67: Demo effectively showcases transitionDuration prop.The component clearly demonstrates the new
transitionDurationprop with four different speeds (150ms, 300ms, 800ms, 1200ms). The responsive grid layout and descriptive headings make the differences easy to compare.Optional: The
defaultContentstring on lines 4-5 is duplicated inscroll-on-open.raw.jsx. If these demos are intended to be reusable, consider extracting the shared Lorem Ipsum text to a common constants file. However, since these are standalone documentation examples, the duplication is acceptable.apps/docs/content/components/accordion/scroll-on-open.raw.jsx (1)
1-65: Excellent side-by-side comparison demonstrating scrollOnOpen.The demo effectively showcases the
scrollOnOpenbehavior through a clear side-by-side comparison. The scrollable containers with spacer content create a realistic scenario where the feature provides value, and the inclusion of the Button components nicely illustrates how interactive elements benefit from auto-scrolling.Optional: The
defaultContentstring (lines 4-5) is duplicated intransition-duration.raw.jsx. Consider extracting to a shared constant if consolidating demo utilities in the future, though the current approach is acceptable for standalone examples.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.changeset/shy-kiwis-bake.md(1 hunks)apps/docs/content/components/accordion/index.ts(2 hunks)apps/docs/content/components/accordion/scroll-on-open.raw.jsx(1 hunks)apps/docs/content/components/accordion/scroll-on-open.ts(1 hunks)apps/docs/content/components/accordion/transition-duration.raw.jsx(1 hunks)apps/docs/content/components/accordion/transition-duration.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/docs/content/components/accordion/transition-duration.ts
- apps/docs/content/components/accordion/scroll-on-open.ts
- apps/docs/content/components/accordion/index.ts
🧰 Additional context used
🧬 Code graph analysis (2)
apps/docs/content/components/accordion/scroll-on-open.raw.jsx (1)
apps/docs/content/components/accordion/transition-duration.raw.jsx (2)
App(3-67)defaultContent(4-5)
apps/docs/content/components/accordion/transition-duration.raw.jsx (1)
apps/docs/content/components/accordion/scroll-on-open.raw.jsx (2)
App(3-65)defaultContent(4-5)
🪛 LanguageTool
.changeset/shy-kiwis-bake.md
[grammar] ~6-~6: Ensure spelling is correct
Context: ... customize animation speed. Defaults to 300ms as it is right now
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
KMNowak
force-pushed
the
feat/accordion_transitionDuration_and_scrollOnOpen
branch
from
c209bd0 to
8c5975f
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/components/accordion/src/accordion-item.tsx (1)
105-116: Consider extracting duration calculation to reduce duplication.The expression
transitionDuration ? transitionDuration / 1000 : 0.3is duplicated in both exit and enter variants. While minor, extracting it improves maintainability.Apply this diff to extract the duration:
+ const duration = transitionDuration ? transitionDuration / 1000 : 0.3; + const transitionVariants: Variants = { exit: { ...TRANSITION_VARIANTS.collapse.exit, overflowY: "hidden", - transition: {duration: transitionDuration ? transitionDuration / 1000 : 0.3}, + transition: {duration}, }, enter: { ...TRANSITION_VARIANTS.collapse.enter, overflowY: "unset", - transition: {duration: transitionDuration ? transitionDuration / 1000 : 0.3}, + transition: {duration}, }, };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/components/accordion/src/accordion-item.tsx(6 hunks)
🔇 Additional comments (6)
packages/components/accordion/src/accordion-item.tsx (6)
3-3: LGTM: Imports are correctly added.The new imports (ReactNode, useRef, useLayoutEffect) are properly used in the implementation.
Also applies to: 6-6
34-35: LGTM: New props are correctly destructured.The scrollOnOpen and transitionDuration props are properly extracted from the hook and used throughout the component.
46-46: LGTM: Content ref is properly initialized.The contentRef is correctly typed and will be attached to content wrapper elements.
89-103: LGTM: Content ref is correctly attached in non-animated paths.Both the
keepContentMountedand conditional render branches properly attachcontentRefto the content wrapper.
132-134: LGTM: Content ref is correctly attached in animated paths.The
contentRefis properly attached to the content wrapper in both thekeepContentMounted(line 132) andAnimatePresence(line 153) render paths, ensuring the scroll effect can access the DOM node.Also applies to: 153-155
161-161: LGTM: Dependency array correctly updated.The
transitionDurationprop is correctly added to theuseMemodependencies since it affects thetransitionVariantsobject.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/components/accordion/src/accordion-item.tsx (1)
109-120: Consider nullish coalescing for clarity.The transition duration logic is correct, but could be slightly clearer by making the default explicit in milliseconds.
Apply this diff for improved clarity:
const transitionVariants: Variants = { exit: { ...TRANSITION_VARIANTS.collapse.exit, overflowY: "hidden", - transition: {duration: transitionDuration ? transitionDuration / 1000 : 0.3}, + transition: {duration: (transitionDuration ?? 300) / 1000}, }, enter: { ...TRANSITION_VARIANTS.collapse.enter, overflowY: "unset", - transition: {duration: transitionDuration ? transitionDuration / 1000 : 0.3}, + transition: {duration: (transitionDuration ?? 300) / 1000}, }, };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/components/accordion/src/accordion-item.tsx(6 hunks)
🔇 Additional comments (5)
packages/components/accordion/src/accordion-item.tsx (5)
3-3: LGTM!The import additions (
useRef,useLayoutEffect) are appropriate for the new scroll functionality.Also applies to: 6-6
34-35: LGTM!The new props are clearly named and appropriately destructured.
46-46: LGTM!The ref is correctly typed for the content wrapper div.
93-97: LGTM!The
contentRefis consistently attached across all render paths (animated vs. non-animated, keepContentMounted vs. conditional rendering), ensuring scroll functionality works in every scenario.Also applies to: 100-106, 136-138, 157-159
165-165: LGTM!The dependency array correctly includes
transitionDuration, ensuring the animation updates when the duration prop changes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/accordion/src/accordion-item.tsx (1)
46-74: Scroll-on-open implementation is solid.The double
requestAnimationFramepattern ensures the layout has updated before scrolling, cleanup properly cancels pending callbacks, and the re-check on line 59 guards against stale refs. The use ofuseLayoutEffectis appropriate for synchronous DOM manipulation.Optional observation: There's a theoretical race where if
isOpentoggles very quickly (~<33ms), the inner RAF callback might execute after the accordion closes but still scroll becauseisOpen(line 59) is captured from the effect's closure rather than reflecting current state. This is extremely unlikely in practice and would only cause a minor visual glitch. If you want to eliminate this edge case entirely, you could storeisOpenin a ref and checkisOpenRef.currentat line 59, but the current approach follows standard React patterns and is acceptable.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/components/accordion/src/accordion-item.tsx(6 hunks)
🔇 Additional comments (6)
packages/components/accordion/src/accordion-item.tsx (6)
3-3: LGTM! Imports are appropriate.The new imports (
useRef,useLayoutEffect) are correctly added for the scroll-on-open functionality, and theReactNodetype import is properly positioned.Also applies to: 6-6
34-35: LGTM! New props correctly destructured.The
scrollOnOpenandtransitionDurationprops align with the PR objectives and are properly destructured from the hook.
91-104: LGTM! ContentRef correctly attached in non-animated branches.Both the
keepContentMountedand conditional rendering paths properly attachcontentRefto the content wrapper div, enablingscrollIntoViewto work when animations are disabled.
107-118: LGTM! Transition duration conversion is correct.The milliseconds-to-seconds conversion (
transitionDuration / 1000) is appropriate for Framer Motion, and the 0.3s default matches the original behavior.
134-136: LGTM! ContentRef correctly attached in animated branches.Both animated render paths properly attach
contentRefto the inner div wrapper (not them.section), ensuringscrollIntoViewworks correctly across all rendering scenarios.Also applies to: 155-157
163-163: LGTM! Dependency array correctly updated.Adding
transitionDurationto the dependency array ensures the content re-renders with updated transition variants when the duration changes.
1 participant
Closes #
📝 Description
transitionDuration(default 300ms just like in Framer), to customize behaviourscrollOnOpento scroll automatically to just opened accordion item to increase content visibility.⛳️ Current behavior (updates)
Currently it was not possible to customize animation duration time.
🚀 New behavior
User can customize animation duration time and additionally apply automatic scroll to just opened accordion item.
💣 Is this a breaking change (Yes/No):
No
📝 Additional Information
Applied changes suggested in original PR: #5126
Summary by CodeRabbit
New Features
Documentation
Tests