Skip to content

Conversation

Arkingco
Copy link
Contributor

Describe your changes

  • 공개 스트링캣 기능을 구현하였습니다
  • 공개 스트링캣 카드뷰의 마우스 가로 애니메이션을 위해 swipe js 라이브러리를 추가하였습니다.
  • layout에 있는 use client 태그를 삭제 하였습니다.

Screen Capture

Issue number and link

@Arkingco Arkingco added the ✨ feature New feature or improvement label May 27, 2024
@Arkingco Arkingco requested review from lamPolar and lyssoi May 27, 2024 05:38
@Arkingco Arkingco self-assigned this May 27, 2024
Copy link
Contributor

@lyssoi lyssoi left a comment

Choose a reason for hiding this comment

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

고생하셨습니다~ 질문 몇개 드렸어요

<div className="m-auto h-full max-w-md font-pretentdard">
<InApp />
<RecoilRoot>
<RecoilWrapper>
Copy link
Contributor

Choose a reason for hiding this comment

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

recoilroot를 recoilwrapper로 변경하신 이유가 있나요?

Copy link
Contributor

Choose a reason for hiding this comment

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

recoilRoot가 하던 일과 동일한 행동을 recoilWrapper에서 하고 있는 것 같아서 저도 궁금합니다!
파일을 분리하신 이유가 있으실까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

layout 파일 상단에 'use client' 를 지우고 next js의 이점인 pre rendering을 사용 하기 위해 RecoilWrapper로 묶었습니다.

use client를 지우면 layout파일에서 내부에서 state와 같은 클라이언트 코드를 사용할 수 없기 때문에
클라이언트 코드들을 layout 파일에서 지워야 합니다.
변경 하기 전 RecoilRoot와 같은 경우 내부에 있는 state 때문에 RecoilRoot를 호출하는 컴포넌트는 무조건 'use client'로
선언되어 있어야 합니다
따라서 RecoilRoot컴포넌트를 RecoilWrapper로 감싸고 그 내부에서 'use client' 를 호출하는 방식으로 해결하였습니다.

아래 설명은 layout에 use client를 제거한 생각의 근거 입니다.

일반적인 next js 프로젝트에서 layout 코드를 서버쪽에서 호출하게 만드는 게 일반적 입니다.
next js 가이드에서도 그렇게 안내 되어 있습니다.
현재 프로젝트 layout.tsx 파일 상단에는 'use client' 로 선언되어 있는데,
이때문에 layout 페이지가 서버에서 pre rendering 되는 게 아닌 클라이언트 쪽에서 rendering 될 것으로 예상 됩니다.
이는 pre rendering을 사용해서 얻는 next js 의 SEO 장점과 웹 페이지 로딩 속도 면에서 빨라지는 이점이 use client를 사용해서 사라지기 때문에 지워야 한다고 생각 들었습니다

title: string,
description: string,
): Promise<boolean> => {
return new Promise((resolve) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

저희가 보통 async await을 사용했었는데, 이건 프로미스로 하신 이유가 있나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
위 함수 내부에서 비동기적인 뭔가를 처리하려고 하는 게 아니라 결과 값 자체를 전달하는 것 이기 때문에 async await는 필요 없습니다.
다만 코드를 호출하는 Dialog에서 async await를 사용하고 있습니다.

Copy link
Contributor

@lamPolar lamPolar left a comment

Choose a reason for hiding this comment

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

기선님 리뷰를 너무 늦게 드려서 죄송합니다!
전체적으로 이상하게 동작할 만한 것은 보이지 않네요! 수고하셨습니다~
다만 몇가지 궁금한 것이 있어서 코멘트 남겼습니다! 답변 기다리겠습니다.
감사합니다~

@@ -1,3 +1,5 @@
'use client';
Copy link
Contributor

Choose a reason for hiding this comment

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

혹시 이부분 추가하신 이유가 있을까요?
이 파일에서 어떤 부분때문에 추가하신 건지 궁금합니다~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

layout 에서 use client를 제거하고 내부 컴포넌트에서 use client를 선언하는 방식으로 변경하게되어 생긴 코드 입니다!

<div className="m-auto h-full max-w-md font-pretentdard">
<InApp />
<RecoilRoot>
<RecoilWrapper>
Copy link
Contributor

Choose a reason for hiding this comment

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

recoilRoot가 하던 일과 동일한 행동을 recoilWrapper에서 하고 있는 것 같아서 저도 궁금합니다!
파일을 분리하신 이유가 있으실까요?

<div className="">
<input
type="checkbox"
className="sr-only"
Copy link
Contributor

Choose a reason for hiding this comment

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

오...sr-only class를 input에 사용하는 경우는 처음보네요!
간단하게 어떤 걸 화면에서 숨기기 위해서 사용하셨는지 여쭤봐도 될까요?

@Arkingco Arkingco requested review from lamPolar and lyssoi July 8, 2024 03:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ feature New feature or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants