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

[Feature] 디자인 시스템 기반 컴포넌트 이어서 작업 #11

Merged
merged 4 commits into from
May 20, 2024

Conversation

hin6150
Copy link
Member

@hin6150 hin6150 commented May 19, 2024

PR Type

  • 기능 추가

Overview

  • List 컴포넌트 구현
  • Icon 컴포넌트 일부 구현
  • SearchBar 컴포넌트 레이아웃 구현

Additional Comment

  • 디자인 시스템에 해당하는 공통 컴포넌트 부분은 List와 Icon 관련된 작업이라고 생각
  • List 컴포넌트의 경우 Page, Search(익스텐션)으로 나눠서 작업
  • Icon은 assets/Icon에서 사용 가능한 기본적인 틀 제작
    • Storybook과 Web에서 사용가능하도록 각각의 Public Folder에 생성
    • 모든 아이콘을 넣을까 하다가 Large 기준으로 대표되는 아이콘만 생성
    • Q. 아이콘 어떻게 사용해야 하는지 이야기를 듣고자 함
  • SearchBar는 작업하다 이 부분은 공통 컴포넌트 보다는 웹쪽에 가까운 것 같아 레이아웃과 간단한 기능만 구현 후 Merge 후 작업 예정

Issue

#7

Check List

  • 관련된 이슈를 연결하였나요?
  • 올바른 PR 컨벤션을 작성했나요?
  • 적절한 라벨을 선택했나요?
  • Assignee와 Reviewer를 설정했나요?

References

@hin6150 hin6150 added the 🧑‍💻 Feature 기능 개발 및 삭제 관련 사항입니다 label May 19, 2024
@hin6150 hin6150 requested a review from cobocho May 19, 2024 15:43
@hin6150 hin6150 self-assigned this May 19, 2024
Copy link
Contributor

@cobocho cobocho left a comment

Choose a reason for hiding this comment

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

고생하셨습니다!!

it('Icon은 정상적으로 렌더링 된다.', () => {
render(<Icon />)

expect(screen.getByText('Icon')).toBeInTheDocument()
Copy link
Contributor

Choose a reason for hiding this comment

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

P3: 이미지의 경우 getByAltText 쿼리를 통해 테스트 할 수 있습니다

Comment on lines 7 to 9
render(<List>Text</List>)

expect(screen.getByText('List')).toBeInTheDocument()
Copy link
Contributor

Choose a reason for hiding this comment

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

P2: 실제 children과 테스트 쿼리가 달라서 테스트가 깨지는 것 같아요!

Comment on lines +8 to +9
variant?: 'page' | 'search'
kind?: 'table' | 'synonym' | 'description' | 'title'
Copy link
Contributor

Choose a reason for hiding this comment

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

P4: css.ts에서 만든 ListVariants를 활용할 수도 있을 것 같아요!

Copy link
Member Author

Choose a reason for hiding this comment

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

List Props으로 ListVariants를 가져오면 와 같이 사용할 수 있는데, 이 경우에는 사용자가 page와 search 두가지 경우 모두를 입력할 수 있게 되어서 구현에 어려움이 있었습니다.

keyof ListVariants으로 key 값을 가져오려고 해도 undefined가 붙어서 그런지 string은 never 형식에 사용할 수 없다는 type 에러가 발생합니다 ㅠㅠ

혹시 제가 놓치고 있는 부분이 있는 것이면 알려주세요!

@hin6150 hin6150 merged commit b26aae6 into main May 20, 2024
3 checks passed
@hin6150 hin6150 deleted the design-system branch May 20, 2024 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧑‍💻 Feature 기능 개발 및 삭제 관련 사항입니다
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants