Skip to content
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

Add accordion component #343

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Add accordion component #343

wants to merge 6 commits into from

Conversation

rose-liang
Copy link
Collaborator

@rose-liang rose-liang commented Aug 15, 2023

Closes: https://vimean.atlassian.net/browse/DSYS-26

What this PR does

creates Accordion component:

  • Accordion and Accordion.Item minor component

Screenshots & Recordings

light mode:
Screenshot 2023-08-22 at 10 38 43 AM

dark mode:
Screenshot 2023-08-22 at 10 38 55 AM

Testing

https://vimeo.github.io/iris/sb/DSYS-26-accordion/?path=/story/components-accordion--controls

@juliewongbandue
Copy link
Collaborator

Hey @rose-liang ! Thanks for taking this on. A few things!:

  • Let's add some accessibility: each section should be accessible via keyboard (tab to section and open with enter or space) and include a focus state. See this pattern for reference. We do have a Focus component, but this can also be achieved with css.
  • I would consolidate the stories that you have under examples/ to one story, with all components and their visual props/states (open, closed, disabled, etc..) displayed there in effort to keep our snapshot quantity reasonable (1 story = 1 snapshot).

Hope that's clear. Let me know if you have any questions!

`;

export const Title = styled(Paragraph)`
font-size: ${rem(16)};
Copy link
Collaborator

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)`
Copy link
Collaborator

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.

src/components/Accordion/Accordion.style.ts Show resolved Hide resolved
src/components/Accordion/Accordion.style.ts Show resolved Hide resolved
src/components/Accordion/Accordion.style.ts Show resolved Hide resolved
Comment on lines +43 to +44
? `${rem(1)} solid ${grayscale(800)}`
: `${rem(1)} solid ${grayscale(50)}`
Copy link
Collaborator

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/)

Co-authored-by: rose-liang <rose-liang@users.noreply.github.com>
@rose-liang rose-liang marked this pull request as ready for review August 18, 2023 19:35
@rose-liang rose-liang requested review from a team as code owners August 18, 2023 19:35
Comment on lines 15 to 25
title: string;
format: 'basic' | 'secondary';
index: number;
allowMultiple: boolean;
defaultActive: boolean;
setActiveIndex: ({ index }) => void;
itemActive: boolean;
subcopy?: string;
icon?: string;
hasError?: boolean;
disabled?: boolean;
Copy link
Collaborator

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:

Screenshot 2023-08-18 at 3 55 41 PM

Comment on lines 63 to 79
<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" />
)}
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants