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.

Sign up for GitHub

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

Jump to bottom

Feat: create accordion #159

Merged
merged 20 commits into from
Aug 30, 2022
Merged

Feat: create accordion #159

merged 20 commits into from
Aug 30, 2022

Conversation

atomictangerine
Copy link
Contributor

@atomictangerine atomictangerine commented Aug 9, 2022 •
edited
Loading

What I did

  • Added the accordion component
  • manually added the props table
  • updated the README for the component
  • added the accordion to the kitchen-sink page
  • added a storybook example

@vercel
Copy link

vercel bot commented Aug 9, 2022 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
wpds-ui-kit ✅ Ready (Inspect) Visit Preview Aug 30, 2022 at 9:51PM (UTC)
wpds-ui-kit-storybook ✅ Ready (Inspect) Visit Preview Aug 30, 2022 at 9:51PM (UTC)

@atomictangerine atomictangerine marked this pull request as ready for review August 16, 2022 01:18
@artmsilva
Copy link
Contributor

Can you write interaction tests?

wp-aberg
wp-aberg reviewed Aug 30, 2022
View reviewed changes
ui/accordion/src/Accordion.tsx Outdated
import { Icon } from "@washingtonpost/wpds-icon";
import * as AccordionPrimitive from "@radix-ui/react-accordion";
import type * as WPDS from "@washingtonpost/wpds-theme";
import {
Copy link
Contributor

@wp-aberg wp-aberg Aug 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We import these only to export them at the end. When the styled versions of components get created the props come along with them without having to bring these along as well.

All that to say I think this is just extra complexity that we're not really getting anything from.

wp-aberg
wp-aberg approved these changes Aug 30, 2022
View reviewed changes
Copy link
Contributor

@wp-aberg wp-aberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good to go, the import and export of Radix types is a little noisy but not a blocker.

@atomictangerine atomictangerine merged commit e5f2c00 into main Aug 30, 2022
@atomictangerine atomictangerine deleted the feat/create-accordion branch August 30, 2022 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@artmsilva artmsilva artmsilva approved these changes

@wp-aberg wp-aberg wp-aberg approved these changes

Assignees

@atomictangerine atomictangerine

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@atomictangerine @artmsilva @wp-aberg