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(Tab): 유닛 테스트 코드를 작성합니다. #156

Merged
merged 2 commits into from
Sep 30, 2024

Conversation

suwonthugger
Copy link
Member

@suwonthugger suwonthugger commented Sep 28, 2024

변경사항

close #155

  • tab 컴포넌트 unit 테스트를 작성했습니다.

    • tab 컴포넌트가 제대로 렌더링 되는지 여부
    • 각 tab을 클릭했을 때 onChange가 제대로 실행되는지 여부
  • ui 관련된 부분은 크로마틱에서 확인 가능하여 제외했습니다.

    • tab 스토리 파일에서, translator 관련한 부분 확인을 위해서 args에 translator를 추가했습니다.
    • translator는 tab의 실제 value를 대체할 텍스트를 나타내는 객체 입니다. ex) 실제 value는 tab1이고, 화면에 보여주고 싶은 text가 탭1이라면 translator = { tab1: '탭1'}와 같은 방식으로 작성하게 끔 코드가 구성되어있습니다.

링크

https://sopt-makers.slack.com/lists/T040QGZF77H/F07LCJ586TS?record_id=Rec07LCJ58NKS

시급한 정도

🏃‍♂️ 보통 : 최대한 빠르게 리뷰 부탁드립니다.

기타 사항

  • 추후 pr 작성 시, 스토리북 변경사항이 있을 때 github action으로 chromatic 배포를 자동으로 하는 워크플로우가 있었으면 하는데, 어떻게 생각하시는지 궁금합니다!
  • unit 테스트를 어느정도 작성 후에 test 코드도 ci 걸어두면 좋을것 같아요!

@suwonthugger suwonthugger self-assigned this Sep 28, 2024
@suwonthugger suwonthugger linked an issue Sep 28, 2024 that may be closed by this pull request
Copy link

height bot commented Sep 28, 2024

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

Copy link

changeset-bot bot commented Sep 28, 2024

⚠️ No Changeset found

Latest commit: d4fde33

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@jungwoo3490
Copy link
Member

jungwoo3490 commented Sep 28, 2024

translator가 필요한 이유가 궁금해요!!
tabItems를 순회하며 keyitem을 넣고 button 내부에 translator[item]을 넣던데, tabItems 요소 값의 중복 값 가능성 때문에 중복값이 key 값으로 들어가는 걸 우회하기 위해서 이렇게 설계한 걸까요?? 아니면 다른 이유가 있는지 궁금합니다!!

유닛테스트 CI는 지금 걸어둔 상태에요!!
image

Chromatic 배포 자동화 워크플로우 작성은 저는 좋다고 생각합니다!! 다른 분들도 찬성하신다면 바로 이슈파고 작업해보겠습니다!!

@suwonthugger
Copy link
Member Author

@jungwoo3490 사실 저도 translator가 구체적으로 어떤 상황에 필요했는지는 잘 모르겠어요...! 스토리에 translator에 추가한 이유는, 스토리에서 translator 적용이 잘 되는지 확인할 수 있게 하게 위해서 입니다.

@Brokyeom
Copy link
Member

https://github.com/sopt-makers/sopt-operation-frontend/blob/dev/src/utils/translator.ts

이거 아마 이 경우 처럼 실제 서버와 통신해야하는 데이터와 UI로 보여줘야 하는 값이 다른 경우에 사용하려는 목적인 것 같아요.

Copy link
Member

@Brokyeom Brokyeom left a comment

Choose a reason for hiding this comment

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

translator 로 보여지는 값을 다르게 한 것은 작은 변화라 해당 PR에서는 넘어가도 괜찮을 것 같습니다.
테스트에 대한 부분은 문제 없어 보여요!

translator prop을 앞으로 남길지 없앨지 고민이 될 것 같은데, 남기게 된다면 translator의 값의 존재 유무에 따라 값이 잘 렌더링 되는지 확인하는 테스트를 추가하는 것이 좋을 것 같아요

@jungwoo3490
Copy link
Member

https://github.com/sopt-makers/sopt-operation-frontend/blob/dev/src/utils/translator.ts

이거 아마 이 경우 처럼 실제 서버와 통신해야하는 데이터와 UI로 보여줘야 하는 값이 다른 경우에 사용하려는 목적인 것 같아요.

Tab에 표시될 텍스트를 서버에서 받아오는 case가 있을지가 궁금하네요... 보통 클라 쪽에서 사전 정의하고 사용하는 경우가 많다고 생각했어요.
그런데 이 말처럼 실제 서버와 통신하는 값과 연관하여 다른 값을 UI로 보여줘야 한다면 필요할 것 같기도 하네요!!

@suwonthugger
Copy link
Member Author

suwonthugger commented Sep 29, 2024

흠 텍스트를 받아오기 보다는 서버에 데이터를 요청할 때 이런 경우가 있을 것 같아요. 탭을 클릭할 때 마다 api 요청을 통해서 데이터를 받아오도록 설계됐다면, 실제로 보여주어야하는 텍스트는 기획 이고, API의 queryString Param 등이 PLAN인 경우가 있을것 같습니다.

  • 근데 사실 이 부분도 굳이 Tab에서 할 필요성이 있을까 싶긴하네요... Tab에는 단순히 탭의 기본기능을 제공하고, translator의 역할을 하는 객체는 tab 컴포넌트를 사용하는 사람이 정의해서 쓰는게 맞지 않나 싶어요. 개인적으로 translator는 Tab의 역할을 넘어선 부가적인 부분이 아닌가 싶어요

@Brokyeom
Copy link
Member

Brokyeom commented Sep 30, 2024

서버에 데이터를 요청할 때 이런 경우가 있을 것 같아요.

이전 어드민에서 말씀하신대로 실제 UI에 보여지는 값과 서버에 요청을 보내야 하는 값이 달라서 링크와 같은 translator를 여러 개 만들어줬거든요!
(e.g. 뷰에서는 '안드로이드', 요청할때는 'AOS') 그래서 당시에 작업할 때 이런 경우를 고려해서 컴포넌트를 설계한 것으로 보여요

그런데 저도 두 분 의견에 동의하는게, 이런 변환은 시스템 레벨에서 제공할 기능은 아니라고 생각해요! 프로덕트에서 Item을 넣을때 사용자 재량으로 변환을 해서 넣는게 시스템의 복잡성을 줄이는 것이라고 생각이 들구요
다만 translator를 살려둔다면, 탭을 전환할때마다(예를들어 어드민에서의 파트 전환 탭) 탭에 넣어준 prop들로 api 호출과 UI 렌더링을 좀 더 쉽게 할 수 있다고 생각되네요 하지만 이걸 뺀다고 해서 프로덕트의 복잡성이 많이 커지지 않을 것 같아서 제거하는게 어떤가 싶습니다.
이 작업은 티켓 따로 따서 작업하시죠! 오늘 회의에서도 이야기 해 봐요

@Brokyeom
Copy link
Member

bump는 patch로 올려주세요! @suwonthugger

@jungwoo3490
Copy link
Member

그런데 저도 두 분 의견에 동의하는게, 이런 변환은 시스템 레벨에서 제공할 기능은 아니라고 생각해요! 프로덕트에서 Item을 넣을때 사용자 재량으로 변환을 해서 넣는게 시스템의 복잡성을 줄이는 것이라고 생각이 들구요

저도 이 부분에 동의하는 바입니다 :)

@Brokyeom Brokyeom merged commit 764be23 into main Sep 30, 2024
2 checks passed
@Brokyeom Brokyeom deleted the feat/#155/tab-unit-test branch September 30, 2024 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(Tab): 유닛 테스트 코드를 작성합니다.
3 participants