Refactor Accordion component #2157
Conversation
| Accordions are used to group similar content and hide or show it depending on user needs or preferences. | ||
| Accordions give users more granular control over the interface and help digest content in stages, rather | ||
| than all at once. | ||
| Accordion are used to group related content into collapsible sections, allowing users to expand or hide |
There was a problem hiding this comment.
I see a bit strange the start of the definition. Maybe we can fit something like to avoid using a singular name ("Accordion") with a plural verb ("are"). How do you see something like:
"The accordion component is a vertical stack of interactive headings used to group related content into collapsible sections, ..."
There was a problem hiding this comment.
Why are we changing the file extension from ts to tsx?
| icon?: string | SVG; | ||
| }; | ||
|
|
||
| type ValidCombinations = BadgeBeforeProps | BadgeAfterProps | NoBadgeProps | AssistiveTextProps | IconProps; |
There was a problem hiding this comment.
I understand the reasoning behind this complex typing but it's a bit overwhelming, maybe over-engineering. Do you see maybe more value (for the user and us) in simplifying the typing and documenting all these restrictions?
There was a problem hiding this comment.
Right now the user can't know all these restrictions by only reading our docs, which I think is not the ideal experience 😞
There was a problem hiding this comment.
Also, some of the possible combinations of those types may lead to unexpected behaviors:
neveris used to try to make certain properties mutually exclusive, but this can lead to confusing behavior when combining props. For example, inAssistiveTextProps,assistiveTextis allowed butstatusLightis forbidden, while inBadgeBeforeProps,assistiveTextandstatusLightare both allowed. This overlap can complicate type inference and developer experience.- Two types handle the
iconproperty (IconPropsandNoBadgeProps) with subtle differences (one requires it, the other leaves it optional) that can lead to accepting unwanted combinations or causing errors if the intent behind each is not clearly understood.
| /** | ||
| * When true, limits the user to single-open section at a time. When false, multiple sections can be opened simultaneously. | ||
| */ | ||
| independent?: true; |
There was a problem hiding this comment.
Remove the ? operator, it is already covered in the NonIndependentProps type (not defined or false are the same here).
| subLabel="Jan, 09 2025" | ||
| icon="heart_plus" | ||
| disabled | ||
| badge={{ position: "after", element: <DxcBadge label="Enterprise" color="green" /> }} |
There was a problem hiding this comment.
This line is throwing a type error!
| @@ -4,8 +4,10 @@ import DxcAccordion from "./Accordion"; | |||
| describe("Accordion component tests", () => { | |||
| test("Renders with correct aria accessibility attributes", () => { | |||
| const { getByRole } = render( | |||
| <DxcAccordion label="Accordion" defaultIsExpanded> | |||
| <div>test-expanded</div> | |||
| <DxcAccordion defaultIndexActive={0}> | |||
There was a problem hiding this comment.
Same error as in the accessibility tests. It happens in more than one place inside the file, I won't repeat the comment so as not to disturb the review!
| )} | ||
| {badge?.position === "before" && !icon && ( | ||
| <StatusContainer subLabel={subLabel}> | ||
| {disabled ? React.cloneElement(badge.element as ReactElement, { color: "grey" }) : badge.element} |
There was a problem hiding this comment.
Can we import cloneElement above as other regular import from "react" instead of using React?
| const id = useId(); | ||
| const colorsTheme = useContext(HalstackContext); | ||
| const { activeIndex, handlerActiveChange, index, independent } = useContext(AccordionContext) ?? {}; | ||
| const isItemExpanded = independent |
There was a problem hiding this comment.
I suggest storing this value with a useMemo to avoid unnecessary re-renders.
| <DxcContainer width="100%" height="100%"> | ||
| <DxcFlex gap="1.5rem"> | ||
| <LeftSideContainer> | ||
| <DxcFlex gap="0.75rem"> |
There was a problem hiding this comment.
I think we can avoid having an extra container by simply adding to the DxcFlex the prop grow={1}.
Checklist
/libdirectory./websiteas needed.Description
Refactor Accordion component based on new design: https://www.figma.com/design/7CanVB5tlgwOeDIlvK8Dyf/Component-exploration-%26-Testing?node-id=1712-4101&m=dev
DxcAccordionGrouphas been removed and now we have only one component:DxcAccordion. This component acts like the old accordion group, with accordion items inside.Several changes:
disabledis now configurable on each accordion item, instead of in the accordion parent (old Accordion group).subLabeladdedindependentadded. This specifies when only one accordion can be expanded or several.