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

refactor: seriesDetailPage 리팩토링 및 관련 컴포넌트 추가, 댓글 컴포넌트 props 수정 #134

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

sooooooongyi
Copy link
Member

📝 Tasks

구현한 사항을 자세하게 기재합니다.

  • seriesDetailPage 구조 개선
  • 댓글 컴포넌트 props 수정

📝 Results

구현한 결과를 첨부합니다.

📝 논의점

논의하고 싶은 부분을 적어주세요.

seriesDetailPage 리팩토링

  • 기존 컴포넌트 + 코드가 섞여있던 seriesDetailPage
    • -> 각 section 별로 컴포넌트화 (컴포넌트로 만든건 리팩토링 다시 할 예정)
  • loading 컴포넌트로 인한 가독성 저하
    • -> if (loading) return <Loading />; 이 코드로 대체함으로써 가독성 개선 (다른 페이지도 바꿀 예정)

댓글 컴포넌트 props 수정

  • 현재 댓글 컴포넌트는 API func를 전달 받고 댓글 컴포넌트 안에서 api 호출중
    • props 드릴링 문제로 우선 페이지가 아니라 컴포넌트에서 직접 import하는걸로 변경했어요.
    • 하지만, 페이지가 아닌 컴포넌트에서 api를 호출한다면 리렌더링 이슈가 발생해야하는데 발생하지 않는것으로 보아 DOM 직접접근으로 사료됨 (이 부분 같이 논의해보아요..! ⭐️)

앞으로 해야할일! (송이)

  • seriesDetailPage 자식 컴포넌트 리팩토링 + 타입스크립트 전환
  • 메인페이지 swiper에 awesome한 라이브러리를 사용해보고 싶어요!!!
  • 먼썹 프로젝트 css 전략 세우기 (class? styled-components?, props?)

@sooooooongyi sooooooongyi self-assigned this Mar 21, 2022
@netlify
Copy link

netlify bot commented Mar 21, 2022

Deploy Preview for monthsub ready!

Name Link
🔨 Latest commit a7ff457
🔍 Latest deploy log https://app.netlify.com/sites/monthsub/deploys/623b4ad692b5960008682a8f
😎 Deploy Preview https://deploy-preview-134--monthsub.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Mar 21, 2022

Deploy Preview for monthsub-dev ready!

Name Link
🔨 Latest commit a7ff457
🔍 Latest deploy log https://app.netlify.com/sites/monthsub-dev/deploys/623b4ad69bae35000aa02342
😎 Deploy Preview https://deploy-preview-134--monthsub-dev.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Member

@yyoooon yyoooon left a comment

Choose a reason for hiding this comment

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

송님 고생많으셨습니다!!!
지금 comment부분 폴더 구조가 이렇게 폴더 안의 폴더 안의 파일로 되어있는데 폴더를 한겹 벗겨내도 좋을 것 같네요..ㅜㅜ
image

제 생각엔
comment
ㄴ index.jsx (기존의 CommentList/index.jsx)
ㄴ CommentForm.jsx
ㄴ CommentItem.jsx

로 구성하고
index.jsx안에서 CommentForm넣고 CommentItem은 그냥 map돌려도 좋을 것 같은데 어케 생각하시나요??

그리고 api는 페이지에서 import해서 props으로 내려주는게 베스트 같은데
대댓글 기능도 있어서 드릴링이 심해질 것 같긴하네요.. 이 부분은 저도 아직 코드가 잘 이해가 안돼서
게더나 디코로 한번 얘기해보면 좋을 것 같아요!!

...props
}: ContentAddLinkProps): ReactElement => (
<StyledLink to={url} {...props}>
<StyledLink to={`/series/${id}/article/write`} {...props}>
Copy link
Member

Choose a reason for hiding this comment

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

송님! ContentAddLink컴포넌트가 SeriesDetailPage뿐만 아니라
WriteListPage(연재중인 시리즈)에도 쓰이는데 이동할 페이지의 url이 달라서
props으로 받게한거였어요!!
image

Copy link
Member Author

Choose a reason for hiding this comment

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

앗 가 WriteListPage에도 쓰이는군요!! 사이드 이펙트를 고려하지 못했네요 ㅠㅠ 😂 이 부분은 원래대로 다시 수정해놓겠습니다!

comment 부분은 전체적으로 다시 구현 방법을 같이 논의해보면 좋을 것 같아요. 사실 api 분석도 안되었고 정책도 몰라서 수정이 어려울 듯 싶네요! 하루 날 잡고 같이 디코로 조인한번 하시죠!

@@ -86,112 +31,55 @@ const SeriesDetailPage = () => {
};

useEffect(() => {
readSeriesDetail();
getInitialData();
Copy link
Member

Choose a reason for hiding this comment

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

빈값 넣어주는 초기값 없애고 데이터 불리기 전엔 loading컴포넌트 나타나게 한거 맞죠??

loading컴포넌트랑 나타나고 없어지게 하는 로직을 provider로 만들어서 app에 적용해주고, 필요한 페이지에서 나타나고 없어지게 하는 함수만 실행해서 컨트롤하는 방법도 있더라고요!! 저희도 도입하면 깔끔해질 것 같아용 ㅎㅎ

Copy link
Member Author

@sooooooongyi sooooooongyi Mar 23, 2022

Choose a reason for hiding this comment

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

여기에 loading context 관련 내용을 남겨주셨군용! 😎
우선, 위 함수는 그저 일관성을 위해 네이밍만 바꾼거예요ㅋㅋㅋ

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants