feature: Accordion component
#189
Conversation
…ed vars and classes
…info, fix aria-expanded accessibility error
|
marlonmarcello
added
enhancement
There was a problem hiding this comment.
Please maintain a standard and lowercase the file name.
|
|
||
| import { Icon, IconDefs, IconSymbol, Tag, Text } from "@local/components" | ||
|
|
||
| import styles from "../accordion.module.scss" |
There was a problem hiding this comment.
Let's use a single stylesheet per component.
| const icon = "chevron" // Define your icon name here | ||
|
|
||
| const handleToggle = (index: number) => { | ||
| active === index ? setActive(null) : setActive(index) |
There was a problem hiding this comment.
Whenever using the current state to determine a new use, please use a function
https://react.dev/reference/react/useState#updating-state-based-on-the-previous-state
| className?: string | ||
| } | ||
|
|
||
| export function Accordion({}: AccordionProps) { |
There was a problem hiding this comment.
Doesn't look like you are using any of the props from AccordionProps, the object is empty
|
|
||
| import styles from "./accordion.module.scss" | ||
| import AccordionItem, { CardsData } from "./component/AccordionItem" | ||
| import cards from "./content.js" |
There was a problem hiding this comment.
I think content should be passed to the component as a prop and not shipped with it, otherwise you can't reuse it.
|
|
||
| <Tag | ||
| ref={contentEl} | ||
| className={`${styles.collapse} ${styles.accordionBody}`} |
There was a problem hiding this comment.
Use the classnames helper to concatenate class names
| {typeof text === "object" && | ||
| Object.values(text).map((p, i) => ( | ||
| <Text | ||
| key={i} | ||
| variant="body-smaller" | ||
| dangerouslySetInnerHTML={{ __html: p.toString() }} | ||
| /> | ||
| ))} |
There was a problem hiding this comment.
Mind explaining this a bit? Why an object instead of an array? Why the p.toString()? Seems like all you need really is an array of strings.
| } | ||
| > | ||
| {typeof text === "string" && ( | ||
| <Text variant="body-smaller" dangerouslySetInnerHTML={{ __html: text }} /> |
There was a problem hiding this comment.
Avoid dangerouslySetInnerHTML
| const contentEl = useRef<HTMLDivElement>(null) | ||
| const { title, text } = card | ||
|
|
||
| if (!card) return null |
There was a problem hiding this comment.
You are de-structuring the card variable on the line above but doing the check after. Doesn't make much sense and I think the line above will throw if card is anything other than an object.
| </button> | ||
| </Tag> | ||
|
|
||
| <Tag |
There was a problem hiding this comment.
This can be just a div no need to use Tag as it won't be replaced by a dynamic value.
My first StoryBook component so there will be missing details.
Improvements:
ReducedMotion
Currently using @media styles and not const { prefersReducedMotion } = useUserPrefs()
Icon Component
I don't think it's implemented it correctly
ReadMe
Possibly lacking detail
TS
Guessing Typescript implementation isn't perfect
Responsiveness
Accordion height updated on click, not window resize
Takes strings or objects and wraps in < p >. More content design thought needed here
Remove DEFAULT_TAG? Seems verbose for how it's used here