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/workbook 35 ] Workbook 랜딩 페이지 단위 컴포넌트 개발 #43

Merged
merged 16 commits into from
Jun 14, 2024

Conversation

soomin9106
Copy link
Collaborator

@soomin9106 soomin9106 commented Jun 12, 2024

🔥 Related Issues

resolve #35
close #35

💜 작업 내용

  • workbook 제목 및 작가 소개 컴포넌트
  • overview 컴포넌트
  • 커리큘럼 미리보기 컴포넌트
  • mock 데이터로 테스트 코드

✅ PR Point

  • workbook/[id]/page.tsx 로 라우팅 변경 했습니다! (/ 로 접근 시, 미들웨어에서 redirect 작업 예정)
  • msw 사용해서 데이터 fetching 구현했습니다.
  • 테스트 코드 작성 진행 중 입니다. (완료)

😡 Trouble Shooting

  • vitest + msw 통해 페이지가 데이터를 가지고 잘 랜더링 되는지 테스트 중에 svg 핸들링에 에러가 있어서 이 부분 확인해봐야 할 듯 싶습니다 😢
    => vitest config plugin 수정으로 해결
  • test 코드 중 데이터 패칭을 못한 경우에 대한 테스트를 진행하고 있었는데, 아래처럼 처리해도 데이터 패칭이 잘 되고 있습니다..
server.use(
     http.get(apiRoutes.workbook.replace(':workbookId', '1'), ({ request }) => {
      return new HttpResponse(null, { status: 500 });
     })
);

👀 스크린샷 / GIF / 링크

스크린샷 2024-06-12 오후 9 53 29

📚 Reference

Copy link
Collaborator

@Happhee Happhee left a comment

Choose a reason for hiding this comment

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

이게 server.use 를 사용안하고 handler에서 데이터 바꿔주면 error로 잘넘어오긴하는데, 나도 함 문제 찾아볼게!

src/app/page.tsx Outdated
<Image
src={"/main_img.png"}
alt={"Workbook landing image"}
width={480}
height={0}
style={{ maxWidth: '480px', height: 'auto' }}
/>

<TitleSection category={"경제"} title={"재태크, 투자 필수 용어 모음집"} editors={["안나포", "퓨퓨", "프레소"]} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기도 모킹해서 가져올수있을까욥..?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

어 이 페이지 workbook 으로 옮겼습니다! 이 페이지 비워져 있는데 커밋이 안되었나보네용! 수정하겠습니다~

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

<div className="flex flex-row">
<div className="">
<h1 className="h1-bold text-black text-[28px]">{title}</h1>
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

요거 빈태그는 왜 있는걸까욥?!

</div>
</div>
<div className="flex flex-row space-x-[8px] mt-[16px]">
<div className="flex">
Copy link
Collaborator

Choose a reason for hiding this comment

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

자식이 하나인데 flex 역할을 하나요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아 2개 이상의 내용이 들어갈 줄 알고 flex 줬나보네요!! 수정할게용!

<div>
<span className="text-text-gray1 sub2-bold">{editor}</span>
<span className="text-text-gray1 sub2-bold"> · </span>
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

혹시 key 설정안해도 오류 안나나요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

오류는 왜 못봤는지 모르겠는데 key 들어가야 될 것 같네용 ㅎㅎ 수정할게욥


export default function TitleSection ({ category, title, editors }: TitleSectionProps) {
return (
<div className="flex flex-col">
Copy link
Collaborator

Choose a reason for hiding this comment

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

대부분의 태그가 div로 되어있는것같습니다..! 시멘틱하게 구성해보면 더 좋을것같아요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아 좋습니다 ㅎㅎ 수정해볼게용!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@@ -0,0 +1,15 @@
interface OverviewSectionProps {
overview: string
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

이렇게 사용할수도 있지만,먼가 api 응답값에서 가져오는거라면 그 타입에서 pick해서 사용할수도 있을것같습니다!

src/app/page.tsx Outdated
@@ -29,19 +30,32 @@ export default function MainPage() {
setIsClient(true);
}, []);

// TBD: msw 로 변경
Copy link
Collaborator

Choose a reason for hiding this comment

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

변경해주세요!! ㅎㅎ


interface CurriculumItemProps {
day: number
item: ICurriculumItem
Copy link
Collaborator

Choose a reason for hiding this comment

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

I는 무엇을 의미하는것일까요..?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아 이거 처음 타입스크립트 할 때 Interface 하고 type 구분하려고 I 붙이는 게 습관이 되어서 ㅋㅋ ㅠㅠ
그래서 I 가 붙어있는데, 저희는 type 거의 안쓰니까 필요 없을 것 같넹요!! 수정할게요

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아 이거 그리고 컴포넌트하고 이름이 똑같아서 I 를 붙였나봐요 ㅠㅠ
이럴 때 어떻게 할까요?! @Happhee

Copy link
Collaborator

Choose a reason for hiding this comment

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

아 그렇군요...! 저는 보통...I는 너무 약어라서 만약 이름이 같다면 CurriculumItemInfo 정도로 지정하는게 더 명확해보일것같긴합니다..!

@soomin9106 저는..근데 type을 좀 쓰고 있어요 ㅠ.ㅠ interface 보다 확장하는게 편해서...흠하하하하

return (
<CurriculumItem key={item.id} day={idx + 1} item={item} />
)
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

한개의 컴포넌트만 return하는거면 return문 없이도 가능하긴할텐데..컨벤션 맞춰야할것같아용..!

return response.data;
},
});
};;
Copy link
Collaborator

Choose a reason for hiding this comment

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

쿼리컨벤션도토요일에 맞춰용..!

@@ -72,7 +78,7 @@ export default function WorkbookPage() {
<CurriculumSection curriculumItems={workbookInfo.articles} />
</>
)}
</div>
</article>
Copy link
Collaborator

Choose a reason for hiding this comment

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

넘좋아요!!!

@Happhee
Copy link
Collaborator

Happhee commented Jun 13, 2024

피드백 반영 완전 빠르다!! 컨벤션 맞춰야 되는 부분은 킵해두고 머지해도 될것가타!!
고생해써 ㅎㅎ

@soomin9106 soomin9106 merged commit d1a957c into develop Jun 14, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants