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

feat: Implement Accordion component with storybook #16 #17

Merged
merged 2 commits into from
May 10, 2024

Conversation

dgd03146
Copy link
Collaborator

Motivation 🤔

  • Implement an Accordion component to improve content organization.
  • Reference: Shadcn UI Accordion.

Key Changes 🔑

  • Developed a new Accordion component with accessibility features.
  • Integrated the component into Storybook.

To Reviewers 🙏🏻

@ebokyung

  • PR Link: feat: Implement Accordion component with Storybook #16

@dgd03146 dgd03146 added the enhancement New feature or request label May 10, 2024
@dgd03146 dgd03146 requested a review from ebokyung May 10, 2024 04:03
@dgd03146 dgd03146 self-assigned this May 10, 2024
Copy link

@ebokyung ebokyung left a comment

Choose a reason for hiding this comment

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

애니메이션이나 UX, SEO까지 고려하신 점 대단하신 것 같아요.
코드 잘 보고 많이 배워갑니다 🙇🏻‍♀️
거정님은 고민의 폭이 넓고 추상화하고 설계하는 것도 정말 잘하시는 게 느껴집니다!!!

제가 도움을 줄 부분이 없어서 ... 필요하시다면 싱글토글 기능을 제안해봅니다ㅎㅎ~

>(({ className, children, ...restProps }: AccordionContentProps, ref?) => {
const { toggle, handleToggle } = useAccordionContext();
const contentRef = useRef<HTMLDivElement>(null);
useBeforeMatch<HTMLDivElement>(contentRef, handleToggle);

Choose a reason for hiding this comment

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

검색어 찾기 기능이군요 👍🏻

Comment on lines +26 to +35
const contentHeight = useExpandableHeight<HTMLDivElement>(contentRef, toggle);

return (
<Box
ref={contentRef || ref}
style={assignInlineVars({
[S.contentHeightVar]: `${contentHeight + 1}px`,
})}
className={S.content}
HIDDEN={toggle ? undefined : 'until-found'}

Choose a reason for hiding this comment

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

display none이 아닌 숨김으로써 UX를 고려한 부분들 너무 좋습니다 👍🏻

Comment on lines +38 to +42
{Children.map(children, (child, index) => (
<div key={index} className={S.contentChild}>
{child}
</div>
))}

Choose a reason for hiding this comment

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

아 children으로 내부 내용을 유연하게 받는 것도 좋은 것 같아요!

Comment on lines +16 to +27
const { toggle, setToggle, handleToggle } = useToggle();

const value = useMemo(
() => ({ toggle, setToggle, handleToggle }),
[handleToggle, setToggle, toggle],
);
return (
<AccoridonContext.Provider value={value}>
<Box ref={ref} className={S.item} {...restProps}>
{children}
</Box>
</AccoridonContext.Provider>

Choose a reason for hiding this comment

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

비즈니스 로직은 여기에 담는 컴파운드패턴도 배워갑니다

Comment on lines +15 to +19
maxHeight: contentHeightVar,
transitionProperty: 'max-height',
transitionDuration: '200ms',
transitionDelay: '100ms',
transitionTimingFunction: 'cubic-bezier(0.37, 0, 0.63, 1)',

Choose a reason for hiding this comment

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

높이로 컨텐츠를 가린 부분 아이디어가 좋고 고민 많이 하신게 느껴지네요
cubic-vezier도 배워갑니다 !!

@dgd03146
Copy link
Collaborator Author

애니메이션이나 UX, SEO까지 고려하신 점 대단하신 것 같아요. 코드 잘 보고 많이 배워갑니다 🙇🏻‍♀️ 거정님은 고민의 폭이 넓고 추상화하고 설계하는 것도 정말 잘하시는 게 느껴집니다!!!

제가 도움을 줄 부분이 없어서 ... 필요하시다면 싱글토글 기능을 제안해봅니다ㅎㅎ~

넵 싱글 토글 기능이나 애니메이션 부분 개선을 위해 더 고려해봐야겠네요 감사합니다!

Copy link
Collaborator Author

@dgd03146 dgd03146 left a comment

Choose a reason for hiding this comment

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

fix: Remove duplicate package

@dgd03146 dgd03146 merged commit 36bc178 into main May 10, 2024
1 check failed
@dgd03146 dgd03146 deleted the feat/accordion branch May 10, 2024 17:49
@dgd03146 dgd03146 restored the feat/accordion branch May 10, 2024 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants