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/8] 공통 모달 컴포넌트 #26

Merged
merged 4 commits into from
Jul 12, 2024
Merged

Conversation

junseublim
Copy link
Collaborator

👀 관련 이슈

공통 모달 컴포넌트

✨ 작업한 내용

  • 공통 모달 컴포넌트 생성했습니다.

🌀 PR Point

  • 네이밍이나 코드상 수정될 부분 있으면 알려주세요!
  • 우선 가운데 뜨는 모달과 아래에 뜨는 모달을 코드 재사용성으로 고려해 하나의 컴포넌트로 했는데, 추후 분리할 필요가 있을 지도 모르겠습니다. 의견 있으시면 알려주세요!

🍰 참고사항

  • 사용 방법은 스토리북 참고 부탁드립니다.
  • 서버 컴포넌트에서 사용은 우선 고려하지 않았습니다. 제 생각으로는 사용하는 컴포넌트를 클라이언트 컴포넌트로 변경해서 사용하면 되지 않을까 싶은데 서버 컴포넌트에서 사용해야하는 이유가 있을까요?

📷 스크린샷 또는 GIF

  • 스토리북에서 화면을 Small Mobile로 변경하면 더 확인하기 싶습니다 ㅎㅎ
스크린샷 2024-07-11 오전 2 10 56

@junseublim junseublim requested a review from hwanheejung July 10, 2024 17:13
@junseublim junseublim self-assigned this Jul 10, 2024
@hwanheejung
Copy link
Member

hwanheejung commented Jul 10, 2024

서버 컴포넌트에서 사용은 우선 고려하지 않았습니다. 제 생각으로는 사용하는 컴포넌트를 클라이언트 컴포넌트로 변경해서 사용하면 되지 않을까 싶은데 서버 컴포넌트에서 사용해야하는 이유가 있을까요?

@junseublim
지금 고민하고 있는 board/[boardId] 페이지는 interaction이 많아서 클라이언트 컴포넌트로 바꾸는게 맞는 것 같습니다. 그런데 서버 컴포넌트가 더 적합한 부분이 있다면(ex. 서버 컴포넌트의 data fetching을 잘 쓰고 있는 경우), 모달때문에 클라이언트 컴포넌트로 바꾸는게 맞는지 모르겠습니다.

@junseublim
Copy link
Collaborator Author

@hwanheejung 님, 클라이언트 쪽 기능이 필요한 부분만 클라이언트 컴포넌트로 분리해서 사용하면 될 것 같아요. 해봐야 알겠지만, board[boardId] 페이지에서도 페이지 컴포넌트는 서버 컴포넌트로 가져가되 interaction이 있는 부분만 클라이언트 컴포넌트로 분리해서 사용할 수 있지 않을까요..?

@hwanheejung
Copy link
Member

@junseublim 일단 최대한 분리하는 방향으로 생각해보겠습니다! Button에 코멘트 남겼는데 그것만 확인해주시고 머지해도 될 것 같아요!

@junseublim junseublim merged commit 8917f45 into develop Jul 12, 2024
@junseublim junseublim deleted the feature/8-modal branch July 12, 2024 14:31
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.

2 participants