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

채팅을 위한 컴포넌트 구현 #164

Merged
merged 15 commits into from
Aug 1, 2024
Merged

채팅을 위한 컴포넌트 구현 #164

merged 15 commits into from
Aug 1, 2024

Conversation

ss0526100
Copy link
Contributor

PR의 목적이 무엇인가요?

채팅을 위한 컴포넌트를 구현

이슈 ID는 무엇인가요?

설명

유저프리뷰(UserPreview)

image

유저프리뷰 모음(UserPreviewList)

image

채팅 프리뷰(ChattingPreview)

image

채팅 메시지(ChatMessage)

image

채팅 미리보기(ChatList)

image

채팅 하단바(ChattingFooter)

image

질문 혹은 공유 사항 (Optional)

공유 필요한 부분 코멘트 남겨놓겠습니다

@ss0526100 ss0526100 requested a review from jaeml06 July 31, 2024 08:21
@ss0526100 ss0526100 self-assigned this Jul 31, 2024
@ss0526100 ss0526100 requested a review from cys4585 July 31, 2024 08:21
@ss0526100 ss0526100 added FE 프론트엔드 관련 이슈입니다. 🌱 기능추가 feature (새로운 기능 구현) labels Jul 31, 2024
@ss0526100 ss0526100 modified the milestone: 3차 스프린트 Jul 31, 2024
@ss0526100 ss0526100 linked an issue Jul 31, 2024 that may be closed by this pull request
4 tasks

decorators: (Story) => {
return (
<div style={{ height: '200px' }}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

부모의 height에 따라 컴포넌트의 사이즈가 변경되는 지 확인하기 위한 부분입니다.

return (
<div css={list({ theme })}>
{chats.map((chat) => (
<ChatMessage key={chat.message + chat.time} {...chat} />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

추후 chatid로 변경할 예정

imageUrl?: string;
}

export default function ChatMessage(props: ChatMessageProps) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

해당 메시지를 하나만 구현해서 내가 작성한 글이면 오른쪽 정렬, 아니면 왼쪽정렬하게 구현했어요.

근데 컴포넌트를 내가 작성한 채팅, 상대방이 작성한 컴포넌트로 분리할 수도 있겠다는 생각도 드네요

이건 차차 흘러가는 부분에 따라 함께 고민해보아요

Copy link
Contributor

Choose a reason for hiding this comment

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

저는 이것도 괜찮다고 생각해요 ui가 다르면 분리하는게 맞을것 같은데 같으니 boolean으로 재사용할 수 있어서 좋은 것 같아요. 저도 고민해보겠습니다

Copy link
Contributor

Choose a reason for hiding this comment

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

저도 하나로 두는 것이 괜찮을 것 같아요~
방향과 색상에만 차이가 있고, 나머지는 모두 통일성이 있어야 하기 때문에 하나의 컴포넌트로 두면 더 관리하기 편할 것 같습니다. 만약 말풍선 UI가 바뀐다면 둘 다 동일하게 바뀔 것으로 자연스레 예측이 되니까요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

어떤 컴포넌트는 Chat이고 어떤 컴포넌트는 Chatting인데 두개를 나누는 기준은 하나의 메세지를 담는 컴포넌트면 Chat, 아니면 대화가 위주가 되면 Chatting이라고 두었습니다.

ChatList는 Chat이 여러 개 있어서 ChatList라고 했어요.

근데 이 컴포넌트가 대화가 위주가 된다는 관점도 있어서 Chatting 이라는 방안이 존재합니다.

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
이 부분 말하는 거에요

Copy link
Contributor

Choose a reason for hiding this comment

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


return (
<div css={footer({ theme })}>
{/* TODO: 현재 Button이 유연하지 않아 html 태그를 사용
Copy link
Contributor Author

Choose a reason for hiding this comment

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

여기도 언급되어 있듯이 버튼이 아직까지는 유연하지 못한 것 같아서 button html element를 사용했어요

@@ -36,3 +36,28 @@ export const formatHhmmToKorean = (hhmm: string, seperator: string = ':') => {

return `${hour}시 ${minute}분`;
};

export const formatHhmmToKoreanWithPrefix = (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

formatHhmmToKorean: 13:00=> 13시
formatHhmmToKoreanWithPrefix: 13:00=> 오후 1:00

이것도 애매한 부분이네요. 어떻게 바꾸면 될지 차차 이야기해보아요

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

@jaeml06 jaeml06 left a comment

Choose a reason for hiding this comment

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

코멘트 달아두었습니다. 채팅 UI 잘 구현해주셨네요. 고생했어요!

@@ -36,3 +36,28 @@ export const formatHhmmToKorean = (hhmm: string, seperator: string = ':') => {

return `${hour}시 ${minute}분`;
};

export const formatHhmmToKoreanWithPrefix = (
Copy link
Contributor

Choose a reason for hiding this comment

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

그러게요. 지금 제가 기능을 구현하고 있는 코멘트에서는 백엔드에서 포맷팅을 처리한 데이터를 넘겨준다고 해서 이것도 통일하는 것도 좋을 것 같아요

const { imageUrl, size, hasCrown = false } = props;
hasCrown;
return (
<Fragment>
Copy link
Contributor

Choose a reason for hiding this comment

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

하나만 반환해도 괜찮지 않을까요? 굳이 Fragment로 감쌀 필요는 없을 것 같아요

Copy link
Contributor

Choose a reason for hiding this comment

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

이것과 별개로 storybook에서 <></>태그를 인식하지 못하는 문제는 해결하긴 해야겠네요

width: ${size || '3.5rem'};
height: ${size || '3.5rem'};

background: url(${imageUrl}), url(${defaultProfile});
Copy link
Contributor

Choose a reason for hiding this comment

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

이부분도 같이 이야기해봐야 할 주제 같네요. 소파가 코멘트 달아주었던 부분 한번 찾아보고 같이 의견 나누어 봅시다. 그리고 지금 svg를 url로 사용할 수 있게 업데이트를 해서 png를 사용하지 않아도 괜찮을 것 같아요

return (
<div css={container}>
<div>
<h2 css={theme.typography.s2}>{title}</h2>
Copy link
Contributor

Choose a reason for hiding this comment

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

이렇게 타입만 필요한 css는 소파가 구현한 대로 바로 주입할지 아니면 분리할지 고민해봐야 겠네요. 저는 타입만 주입하면 소파방식대로 작성해도 좋을 것 같아요

Copy link
Contributor

Choose a reason for hiding this comment

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

theme.typography.s2 자체는 emotion css 함수이기 때문에, 제 생각에도 소파가 구현한 방식대로 바로 주입하는 것도 괜찮을 것 같네요~

Copy link
Contributor

Choose a reason for hiding this comment

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

imageUrl?: string;
}

export default function ChatMessage(props: ChatMessageProps) {
Copy link
Contributor

Choose a reason for hiding this comment

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

저는 이것도 괜찮다고 생각해요 ui가 다르면 분리하는게 맞을것 같은데 같으니 boolean으로 재사용할 수 있어서 좋은 것 같아요. 저도 고민해보겠습니다

Copy link
Contributor

@cys4585 cys4585 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다! 컴포넌트를 적절히 잘 해주신 것 같아요. 디자인 요구사항도 어려웠을텐데 잘 반영해주셨네요!

모든 컴포넌트가 components 디렉토리 하위에 나열되어 있는데, 도메인 별로 묶을 필요가 있을 것 같아요.
폴더링 규칙에 대해 의견을 나누는 시간을 가지면 좋겠습니다! @jaeml06

Comment on lines +8 to +10
interface ChatListProps {
chats: ChatMessageProps[];
}
Copy link
Contributor

Choose a reason for hiding this comment

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

ChatListProps의 chats에 ChatMessageProps[]가 들어가는게 좀 특이한데, 아직 서버와 chat 데이터 구조가 정해지지 않아서 임시로 정해둔걸까요?

그 데이터 구조를 type으로 선언해두고 chats: Chat[]로 interface를 선언하는게 자연스럽다 생각이 듭니다!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

네 아직 서버에서 오는 값이 정해지지 않아서 이렇게 놔뒀어요

값이 정해진다면 업데이트하겠습니다

imageUrl?: string;
}

export default function ChatMessage(props: ChatMessageProps) {
Copy link
Contributor

Choose a reason for hiding this comment

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

저도 하나로 두는 것이 괜찮을 것 같아요~
방향과 색상에만 차이가 있고, 나머지는 모두 통일성이 있어야 하기 때문에 하나의 컴포넌트로 두면 더 관리하기 편할 것 같습니다. 만약 말풍선 UI가 바뀐다면 둘 다 동일하게 바뀔 것으로 자연스레 예측이 되니까요!

Comment on lines 13 to 22
export interface ChatMessageProps {
sender: string;
message: string;
isMyMessage: boolean;
time: string;
imageUrl?: string;
}

export default function ChatMessage(props: ChatMessageProps) {
const { sender, message, isMyMessage, time, imageUrl } = props;
Copy link
Contributor

Choose a reason for hiding this comment

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

컴포넌트 이름으로 그냥 Chat은 어떨까요? 지금 ChatMessage를 사용하는 상위 컴포넌트가 ChatList인 만큼 컴포넌트의 관계를 더 예측하기 편할 것 같습니다 :)

return (
<div css={container}>
<div>
<h2 css={theme.typography.s2}>{title}</h2>
Copy link
Contributor

Choose a reason for hiding this comment

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

theme.typography.s2 자체는 emotion css 함수이기 때문에, 제 생각에도 소파가 구현한 방식대로 바로 주입하는 것도 괜찮을 것 같네요~

@ss0526100 ss0526100 merged commit 058d776 into develop-frontend Aug 1, 2024
1 check passed
@ss0526100 ss0526100 deleted the feat/#139 branch August 1, 2024 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FE 프론트엔드 관련 이슈입니다. 🌱 기능추가 feature (새로운 기능 구현)
Projects
Status: 완료
Development

Successfully merging this pull request may close these issues.

채팅 컴포넌트 구현
3 participants