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

chat main component (after #4 merged) #6

Open
wants to merge 28 commits into
base: dev
Choose a base branch
from
Open

chat main component (after #4 merged) #6

wants to merge 28 commits into from

Conversation

jiyong1
Copy link
Member

@jiyong1 jiyong1 commented Dec 7, 2021

개요

chat main component와 필요한 이외의 common 컴포넌트도 작성하였습니다.

이슈 번호

변경사항

  • x 버튼과 뒤로 가기 버튼 컴포넌트를 작성하였습니다.
  • 프로젝트 이름과 설명 그리고 피드백을 볼 수 있는 메인 컴포넌트를 작성완료 하였습니다.

특이사항

@jiyong1 jiyong1 added the feature New feature or request label Dec 7, 2021
@jiyong1 jiyong1 added this to the 9주차 milestone Dec 7, 2021
@jiyong1 jiyong1 self-assigned this Dec 7, 2021
@jiyong1 jiyong1 changed the title chat main component chat main component (after #3 merged) Dec 7, 2021
@jiyong1 jiyong1 changed the title chat main component (after #3 merged) chat main component (after #4 merged) Dec 7, 2021
@Seogeurim Seogeurim linked an issue Dec 12, 2021 that may be closed by this pull request
1 task
Copy link
Member

@pumpkiinbell pumpkiinbell left a comment

Choose a reason for hiding this comment

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

수고하셨습니다~~

Comment on lines +35 to +37
export const mainInfoStyle = css`
/* padding: 0 1.5rem; */
`;
Copy link
Member

Choose a reason for hiding this comment

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

이 부분 지워도 되지 않을까요..!?

Copy link
Member Author

Choose a reason for hiding this comment

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

앗 지우겠습니다 ㅎㅎ..

Comment on lines +52 to +54
display: -webkit-box;
-webkit-box-orient: vertical;
-webkit-line-clamp: 3;
Copy link
Member

Choose a reason for hiding this comment

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

webkit 속성을 사용하신 이유가 궁금하네요 :)

Copy link
Member Author

Choose a reason for hiding this comment

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

음.. 라인수를 제한시키려면 저렇게 해야한다고 해서욥.. ㅎㅎ

Copy link
Member

@pumpkiinbell pumpkiinbell Dec 31, 2021

Choose a reason for hiding this comment

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

표준이 아니다보니 엣지나 파폭은 안먹힐것 같아서요 ㅠㅠ
라인수 제한을 꼭 해야 하는 걸까용??

Comment on lines +16 to +19
<path
d="M75 10 L 25 50 L 75 90"
css={[{ stroke: color }, backPathDefaultStyle]}
/>
Copy link
Member

Choose a reason for hiding this comment

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

오 path 태그로 구현하셨군요... 대박

color: string;
}

const Exit: FC<ButtonProps> = ({ width, height, color }) => {
Copy link
Member

Choose a reason for hiding this comment

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

피그마에 있는 svg, png 파일이 아닌 이렇게 직접 구현하신 이유가 궁금합니다!?

Copy link
Member Author

Choose a reason for hiding this comment

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

한번 해보고 싶었습니다.. 간단한 path니깐요 ㅋㅋㅋㅋ

Comment on lines -5 to -22
colorPrimary: string;
colorPrimaryDark: string;
colorPrimaryLight: string;
colorSub: string;
colorSubLight: string;
colorBg: string;
colorWhite: string;
colorGray1: string;
colorGray2: string;
colorGray3: string;
colorIndigo: string;
colorBrown: string;
colorSuccess: string;
colorError: string;

fontBasic: string;
fontCoding: string;
fontCodingBold: string;
Copy link
Member

Choose a reason for hiding this comment

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

기존에 있는 타입을 지우고 인덱스 시그니처를 사용하신 이유가 있을까요??
이전과 달리 Theme가 너무 큰 범위의 프로퍼티 타입을 가지게 되는것 아닐까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

이건 이전에 수정할 때 상황에 따라 theme이 달라지도록 하려다보니 일단 바꿨습니다..

Copy link
Member

@Seogeurim Seogeurim left a comment

Choose a reason for hiding this comment

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

혹시 지난 PR dev merge 후 변경사항이 아직 남아있는 것 같은데, rebase 한 번만 해주실 수 있을까요?!!

Copy link
Member

@Seogeurim Seogeurim left a comment

Choose a reason for hiding this comment

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

dev rebase는 확인해보니 되어있는데 이전 swc 관련 변경사항들이 보이는 것 같네요!! 무슨 문제일까용? 👀
확실히 storybook 보니까 PR 리뷰 하면서 스스로 테스트해본걸 storybook 통해서 쉽게쉽게 볼 수 있어서 정말 좋네요 !!! ㅎㅎㅎ
지난 회의 시간에 컴포넌트 분리 기준에 대해 말씀해주신게 왜 고민이 되었는지 이해가 되네요!!!
몇가지 코멘트 남겨보았습니닷 👍🏻

Comment on lines +11 to +16
// @TODO: primary 정보를 가져와 어떤 theme을 전달할지 결정해야 합니다.
const isPrimary = true;
return (
<BrowserRouter>
<RecoilRoot>
<ThemeProvider theme={isPrimary ? primaryTheme : subTheme}>
Copy link
Member

Choose a reason for hiding this comment

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

ThemeProvider 좋네용 !!! 👍🏻👍🏻👍🏻

Comment on lines +10 to +11
const ProjectIntro: FC<ProjectIntroProps> = ({ name, description }) => {
// @TODO: 버튼 클릭시 채팅 컴포넌트로 넘어가도록 해야합니다.
Copy link
Member

Choose a reason for hiding this comment

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

어떤 버튼 클릭시인가요?

Comment on lines +5 to +8
const TelbbyServiceClient: FC = () => {
// @TODO 여기에서 Service Client를 구현해나갑니다.
return <div css={chatMainDefaultStyle} />;
};
Copy link
Member

Choose a reason for hiding this comment

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

TelbbyServiceClient 컴포넌트를 Main 컴포넌트 밑에 위치시킨 이유는 무엇일까요?

color: string;
}

const Exit: FC<ButtonProps> = ({ width, height, color }) => {
Copy link
Member

Choose a reason for hiding this comment

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

BackBtn인데 Exit이라고 네이밍을 잘못 해둔 것 같네요!!
그리고 Main 밑에는 BottomButton 이라고 버튼을 풀네임으로 쓰고, common 밑에는 BackBtn, ExitBtn으로 줄여서 써서 이 부분 통일하면 좋을 것 같아요!

Comment on lines +28 to +32
LightExitButton.decorators = [
(Story) => {
return (
<div style={{ backgroundColor: theme.colorPrimary }}>
<Story />
Copy link
Member

Choose a reason for hiding this comment

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

오호 이렇게도 가능하군요!!!

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

Successfully merging this pull request may close these issues.

Telbby main (프로젝트 정보) 컴포넌트 마크업
3 participants