Added accordion component to ui kit - #293
Conversation
rushi
left a comment
There was a problem hiding this comment.
Also add a Storybook story in src/stories
Summary
WalkthroughChangesAccordion feature
Sequence Diagram(s)sequenceDiagram
participant User
participant Accordion
participant Disclosure
participant Body
User->>Accordion: Activates Header
Accordion->>Disclosure: Updates disclosure state
Disclosure-->>Accordion: Returns open state
Accordion->>Body: Renders animated visibility
Accordion-->>User: Rotates chevron and displays Body
Merge Risk: 🔵 Low · up to The PR adds and exports an Accordion component, but its story currently ignores the body styling control, its button markup has a semantic/accessibility concern, and prop validation is not registered correctly. It is mergeable with explicit owner follow-up for these bounded issues. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ce7b46e7-2223-417b-b3fe-1767de1f1e0b
📒 Files selected for processing (3)
src/components/Accordion/Accordion.jsxsrc/index.jssrc/stories/DataDisplay/Accordion.stories.js
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
xola/x2-seller(manual)
Included review availability: 1 review is currently available. Based on recent review activity, included reviews refill at 5 per hour.
| <Disclosure.Button className="flex items-center"> | ||
| <div className="mr-2" data-testid="accordion-dropdown-icon"> | ||
| <ChevronRightIcon className={clsx(open && "rotate-90")} /> | ||
| </div> | ||
|
|
||
| {childrenArray.filter((child) => child?.type === Header)} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- Accordion.jsx ---'
cat -n src/components/Accordion/Accordion.jsx
printf '%s\n' '--- related Accordion files/stories ---'
fd -i 'Accordion' src
printf '%s\n' '--- Disclosure.Button usage ---'
rg -n -C 3 'Disclosure\.Button|data-testid="accordion-dropdown-icon"' srcRepository: xola/ui-kit
Length of output: 3652
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- Accordion story ---'
cat -n src/stories/DataDisplay/Accordion.stories.js
printf '%s\n' '--- Headless UI dependency ---'
rg -n -C 2 '`@headlessui/react`|headlessui' package.json yarn.lock package-lock.json pnpm-lock.yaml 2>/dev/null || true
printf '%s\n' '--- Structural verifier ---'
python3 - <<'PY'
from pathlib import Path
import re
path = Path("src/components/Accordion/Accordion.jsx")
source = path.read_text()
button = re.search(r"<Disclosure\.Button\b[^>]*>(.*?)</Disclosure\.Button>", source, re.S)
assert button, "Disclosure.Button block not found"
content = button.group(1)
# The Header component is rendered by the expression inside this button.
header = re.search(r"const Header\b.*?return \(\s*(<div\b.*?</div>)\s*\);", source, re.S)
assert header, "Header wrapper not found"
print("Disclosure.Button uses the default element:", "button")
print("Direct JSX div descendants in Disclosure.Button:", len(re.findall(r"<div\b", content)))
print("Header wrapper element:", re.match(r"<([A-Za-z][\w.]*)\b", header.group(1)).group(1))
print("Header rendered inside Disclosure.Button:", "childrenArray.filter((child) => child?.type === Header)" in content)
assert re.search(r"<div\b", content), "Expected icon div was not found"
assert header.group(1).startswith("<div"), "Expected Header div was not found"
PYRepository: xola/ui-kit
Length of output: 4175
Use phrasing elements inside Disclosure.Button.
Replace the icon wrapper and Header wrapper div elements with span elements. Keep the existing Tailwind classes.
| export const Default = ({ children, heading, headingClass, content }) => { | ||
| return ( | ||
| <Accordion> | ||
| <Accordion.Header className={headingClass}> | ||
| { heading } | ||
| </Accordion.Header> | ||
| <Accordion.Body> | ||
| { content } | ||
| </Accordion.Body> | ||
| </Accordion> | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Pass contentClass to Accordion.Body.
The story declares contentClass in its args and controls, but Default ignores it. The body class control has no effect.
Proposed fix
-export const Default = ({ children, heading, headingClass, content }) => {
+export const Default = ({ children, heading, headingClass, content, contentClass }) => {
return (
<Accordion>
<Accordion.Header className={headingClass}>
{ heading }
</Accordion.Header>
- <Accordion.Body>
+ <Accordion.Body className={contentClass}>
{ content }
</Accordion.Body>
</Accordion>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const Default = ({ children, heading, headingClass, content }) => { | |
| return ( | |
| <Accordion> | |
| <Accordion.Header className={headingClass}> | |
| { heading } | |
| </Accordion.Header> | |
| <Accordion.Body> | |
| { content } | |
| </Accordion.Body> | |
| </Accordion> | |
| ) | |
| export const Default = ({ children, heading, headingClass, content, contentClass }) => { | |
| return ( | |
| <Accordion> | |
| <Accordion.Header className={headingClass}> | |
| { heading } | |
| </Accordion.Header> | |
| <Accordion.Body className={contentClass}> | |
| { content } | |
| </Accordion.Body> | |
| </Accordion> | |
| ) |
No description provided.