-
Notifications
You must be signed in to change notification settings - Fork 1
✨ 632 add accordion component #634
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Tip For best results, initiate chat on the files or code changes. @lehju, hello! 👋 How can I assist you today? Feel free to ask me to review the code, explain something, generate tests, or help with any other task related to this pull request. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughIntroduces a new Accordion component system comprising MucAccordion (parent) and MucAccordionItem (child) components with full state management and event handling. Includes Storybook stories for both components and exports them through index barrel files for public use. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Areas requiring attention:
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/components/Accordion/MucAccordion.vue (1)
70-76: Consider preventing duplicate IDs in multiple mode.When
multipleis true, theonOpenfunction pushes the ID toactiveItemswithout checking if it already exists. While this might not cause immediate issues, it could lead to duplicate entries ifonOpenis called multiple times for the same item.🔎 View suggested improvement
const onOpen = (id: string) => { if (multiple) { - activeItems.value.push(id); + if (!activeItems.value.includes(id)) { + activeItems.value.push(id); + } } else { activeItems.value = [id]; } };src/components/Accordion/MucAccordionItem.vue (1)
124-147: Height animation logic is functional but could be more robust.The
setTimeout(..., 0)pattern for forcing reflow is a known technique for CSS transitions, and the implementation is correct. However, there are a few optional improvements to consider:
- The nested conditionals with
section.valuechecks could be simplified- Using
requestAnimationFrameinstead ofsetTimeout(..., 0)is more semantically correct for DOM updates💡 Optional refactoring idea using requestAnimationFrame
const toggleCollapsable = () => { - if (section.value) { + if (!section.value) return; + collapsed.value = !collapsed.value; if (collapsed.value) { section.value.style.height = section.value.scrollHeight + "px"; collapsing.value = true; emit("close", id); - setTimeout(() => { - if (section.value) { - section.value.style.height = "0"; - } - }, 0); + requestAnimationFrame(() => { + requestAnimationFrame(() => { + if (section.value) { + section.value.style.height = "0"; + } + }); + }); } else { section.value.style.height = "0"; collapsing.value = true; emit("open", id); - setTimeout(() => { - if (section.value) { - section.value.style.height = section.value.scrollHeight + "px"; - } - }, 0); + requestAnimationFrame(() => { + requestAnimationFrame(() => { + if (section.value) { + section.value.style.height = section.value.scrollHeight + "px"; + } + }); + }); } - } };The double
requestAnimationFrameensures the browser has painted the initial state before applying the transition.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/components/Accordion/MucAccordion.stories.ts(1 hunks)src/components/Accordion/MucAccordion.vue(1 hunks)src/components/Accordion/MucAccordionItem.stories.ts(1 hunks)src/components/Accordion/MucAccordionItem.vue(1 hunks)src/components/Accordion/index.ts(1 hunks)src/components/index.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/Accordion/MucAccordion.stories.ts (3)
src/components/Accordion/index.ts (1)
MucAccordion(4-4)src/components/index.ts (1)
MucAccordion(31-31)src/components/Accordion/MucAccordionItem.stories.ts (1)
Template(20-37)
🔇 Additional comments (8)
src/components/index.ts (1)
1-1: Nice addition! The imports and exports follow the existing pattern perfectly.The new Accordion components are correctly imported from the barrel and re-exported in the public API, maintaining consistency with the rest of the codebase.
Also applies to: 31-32
src/components/Accordion/index.ts (1)
1-4: Perfect barrel export! This follows the established pattern in the codebase.Clean and straightforward re-export of the Accordion components.
src/components/Accordion/MucAccordion.vue (1)
1-27: Template structure looks great!The component structure follows Vue 3 best practices with proper semantic HTML and ARIA-friendly setup. The slot pattern for passing state and handlers to children is a solid approach for this accordion architecture.
src/components/Accordion/MucAccordionItem.vue (3)
1-50: Template implementation looks solid!Great use of ARIA attributes for accessibility (
aria-expanded,aria-controls,aria-labelledby), and the conditional icon rendering based on collapse state is clean. The class bindings for transition states are well-structured.
105-119: Watch may not trigger onactiveItemsarray mutations.The watch is monitoring the
activeItemsreference, but since it's passed as a prop and mutated externally (via push/filter in the parent), Vue's reactivity should handle this. However, the watcher would be more explicit if it useddeep: trueor watched the array's contents specifically.That said, since the parent component replaces the array reference in
onClose(filter creates a new array) and inonOpenwhen not in multiple mode, this should work in practice.Consider verifying the reactivity works as expected when:
- Items are added in multiple mode (parent pushes to array)
- Items are removed (parent filters array, creating new reference)
You could add a simple test or manually verify in Storybook that opening/closing items in both single and multiple modes works correctly.
159-172: Lifecycle hooks are properly implemented!Nice job cleaning up the event listener in
onBeforeUnmountand initializing the collapsed state based onactiveItemsinonMounted. This prevents memory leaks and ensures correct initial state.src/components/Accordion/MucAccordion.stories.ts (2)
47-103: Template story demonstrates the component beautifully!The example shows proper usage with the slot scope pattern, including the
defaultItemprop to have the first item open initially. All four accordion items are correctly wired with the shared state.
105-161: Multiple story effectively showcases the multi-expand feature!This story clearly demonstrates the
multipleprop behavior, allowing several accordion items to be open simultaneously. Well done!
FabianWilms
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All else is looking mighty fine!
Co-authored-by: Fabian Wilms <10800158+FabianWilms@users.noreply.github.com>
Description
Add accordion component.
Reference
Issues #632
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.