-
Notifications
You must be signed in to change notification settings - Fork 21
Add accordion component #343
base: main
Are you sure you want to change the base?
Conversation
797e96a
to
6160a88
Compare
6160a88
to
da73fec
Compare
Hey @rose-liang ! Thanks for taking this on. A few things!:
Hope that's clear. Let me know if you have any questions! |
`; | ||
|
||
export const Title = styled(Paragraph)` | ||
font-size: ${rem(16)}; |
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.
Should this be a Header component?
margin-bottom: 0; | ||
`; | ||
|
||
export const Subcopy = styled(Paragraph)` |
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.
Same goes for here, would this technically be a subheader? If so, the header component should also be used. Size and weight can be modified using the variant
and size
props. See this header story for example.
? `${rem(1)} solid ${grayscale(800)}` | ||
: `${rem(1)} solid ${grayscale(50)}` |
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.
If the border color is based on the surface token, you could hold that value in a const (or css property!). You should be able to use the --
css custom property syntax (see example from https://styled-components.com/)
151177c
to
3719de9
Compare
3719de9
to
3700fb8
Compare
Co-authored-by: rose-liang <[email protected]>
title: string; | ||
format: 'basic' | 'secondary'; | ||
index: number; | ||
allowMultiple: boolean; | ||
defaultActive: boolean; | ||
setActiveIndex: ({ index }) => void; | ||
itemActive: boolean; | ||
subcopy?: string; | ||
icon?: string; | ||
hasError?: boolean; | ||
disabled?: boolean; |
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.
Let's leave some comments to describe these props. You can see examples of how some other props were documented here: https://github.com/vimeo/iris/blob/main/src/components/Button/Button.types.ts#L40-L47, this way, the descriptions will render in our storybook args tables:
<HeaderContainer> | ||
{hasError && <CircleWarningIcon />} | ||
{!hasError && icon && icon} | ||
<TitleContainer> | ||
<Title size="4">{title}</Title> | ||
{subcopy && ( | ||
<Subcopy size="6" variant="thin"> | ||
{subcopy} | ||
</Subcopy> | ||
)} | ||
</TitleContainer> | ||
</HeaderContainer> | ||
{isActive ? ( | ||
<ChevronUp width="24" /> | ||
) : ( | ||
<StyledChevronDown width="24" /> | ||
)} |
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.
My concern about this here is what if we want want a different composition of of where the chevron is? What if we just want to render a different kind of warning icon? We should allow devs to compose the contents here however they might need to.
52e694e
to
dd790df
Compare
…o DSYS-26-accordion
dd790df
to
e58dcaa
Compare
Closes: https://vimean.atlassian.net/browse/DSYS-26
What this PR does
creates
Accordion
component:Accordion
andAccordion.Item
minor componentScreenshots & Recordings
light mode:
dark mode:
Testing
https://vimeo.github.io/iris/sb/DSYS-26-accordion/?path=/story/components-accordion--controls