[#92] 마이페이지 6-2 by chan0310 · Pull Request #109 · DaITssu/daitssu-client

chan0310 · 2023-09-26T03:11:46Z

Issue

Resolves [feat] 마이페이지 2부분 작성 #94

Description

-마이페이지 6-2 페이지 전체 구현 완료

Check List

PR 제목을 커밋 규칙에 맞게 작성
PR에 해당되는 Issue를 연결 완료
적절한 라벨 설정
작업한 사람 모두를 Assign
작업한 팀에게 Code Review 요청 (Reviewer 등록)
main 브랜치의 최신 상태를 반영하고 있는지 확인

Screenshot

ChoiSangwon

우선 간단하게 봤는데, 커밋 하나에 코드 2283줄은 너무 가독성이 떨어지네요...
제가 본건

필요없는 주석이 너무 많은 것 같습니다.
styled-component가 설치되고 사용되는 것 같습니다. 저희프로젝트는 emotion만 사용합니다
html태그의 style속성이 너무 길어지는게 많습니다. 이럴경우에는 별도의 스타일로 분리해서 사용하는게 나을 것 같습니다.
커밋은 최대한 작게 작성해주셔야 할것 같습니다..
스토리북 어디갔어요...

양이 너무 많아서 추가적으로 보고 피드백할 부분 계속 작성하겠습니다

ChoiSangwon · 2023-09-26T08:05:21Z

    "react-router-dom": "6.15.0",
    "recoil": "0.7.7",
    "recoil-persist": "5.1.0",
+    "styled-components": "^6.0.8",


..? 저희 스타일드 컴포넌트 사용하지 않고 이모션쓰고있지 않나요?

ChoiSangwon · 2023-09-26T08:11:49Z

+import { COLORS } from '@/styles/constants/colors';
+import styled from '@emotion/styled';
+
+// 가독성을 위해 스타일 파일은 별도로 둡니다.


이런 주석은 필요 없을것 같네요

ChoiSangwon · 2023-09-26T08:12:11Z

@@ -0,0 +1,21 @@
+import React from 'react';
+import { COLORS } from '@/styles/constants/colors';
+// HorizontalLine.js


여기도 주석 필요 없을거 같아요

ChoiSangwon · 2023-09-26T08:12:23Z

+import styled from '@emotion/styled';
+import { TEXT_STYLES } from '@/styles/constants/textStyles';
+
+// 가독성을 위해 스타일 파일은 별도로 둡니다.


ChoiSangwon · 2023-09-26T08:17:42Z

+  formData.subject= new Subject("웹프로그래밍 (가)","2028년 19학기-34",1241523);
+  formData.subject.add_assign_data("과제1",new Date(2023,4,5),new Date(2023,12,3),new Date(2023,12,29),"information","cor",10,8)
+  formData.subject.add_assign_data("과제2",new Date(2023,4,5),new Date(2223,3),new Date(2023,12,29),"information","cor",10,8)
+  formData.subject.add_assign_data("기말고사 공지",new Date(2023,4,5),new Date(2223,3),new Date(2023,12,29),text_m,"cor",10,8)


이렇게 바로 사용하는것 보다는 init()함수 같은것을 만들어서 컴포넌트가 불러와질때 실행되는게 좋을 것 같네요

ChoiSangwon · 2023-09-26T08:18:39Z

+        justifyContent: "space-between", alignItems: "center"}}>
+            <img src="/assets/icon/Nav/calendar_off.svg" style={{marginTop:"2px",
+            alignSelf: "flex-start", 
+            width: "20px", marginRight:"4px"}}/>


html 태그에 속성이 많이 달릴때에는 다 쓰는것 보다는 이모션 사용해서 스타일정의하는게 좋을것 같습니다

ChoiSangwon · 2023-09-26T08:19:00Z

+import React from 'react';
+import { COLORS } from '@/styles/constants/colors';
+// HorizontalLine.js
+import styled from 'styled-components';


왜 스타일드 컴포넌트가 나올까요...

emotion을 사용해야할 것 같습니다

ChoiSangwon · 2023-09-26T08:20:52Z

+export const MyAssignTitleBox = styled.div<Props>`
+  display: flex;
+  flex-direction: column; /* 세로로 요소를 나열합니다. */
+  justify-content: center; /* 세로 중앙 정렬을 수행합니다. */


ChoiSangwon · 2023-09-26T08:22:09Z

+            style={{marginTop:"5px"}}> 차시
+            </style.MyAssignTitleBoxText>
+          </th>
+          <th style={{width:"80px",height:"37px",textAlign:"center",backgroundColor:COLORS.grayscale.Gray4,}}>


이쪽 부분도 너무 속성을 많이 사용하네요. 별도 스타일로 분리해서 만들어도 될것 같아요

halfmoon-mind

현재 PR에 너무 큰 feature에 1개의 커밋에 올라가니 한 눈에 보기 어렵네요
각 페이지 별로 PR을 분리해서 올리는건 어떤가요?

halfmoon-mind · 2023-09-27T06:43:01Z

+  width: ${(props) => (props.width ? `${props.width}px` : '18px')};
+  height: ${(props) => (props.height ? `${props.height}px` : '18px')};


props로 width나 height 값들을 따로 받아오는 이유가 있나요?

halfmoon-mind · 2023-09-27T06:44:01Z

@@ -0,0 +1,26 @@
+import { SubmitBox,SubmitBoxButton,SubmitBoxTitle } from "./Header.styles";


Suggested change

import { SubmitBox,SubmitBoxButton,SubmitBoxTitle } from "./Header.styles";

import * as styles from "./Header.styles";

기존에는 이런 방식으로 불러오고 아래에서는 <styles.SubmitBox>로 호출해주는게 저희 기존 코드들에서 사용했던 방식이라 동일하게 적용해주면 좋을 것 같아요

halfmoon-mind · 2023-09-27T06:44:31Z

+import React from 'react';
+import { COLORS } from '@/styles/constants/colors';
+// HorizontalLine.js
+import styled from 'styled-components';


emotion을 사용해야할 것 같습니다

halfmoon-mind · 2023-09-27T06:44:46Z

+import styled from '@emotion/styled';
+import React from 'react'; // React import 추가
+
+interface Props {


조금 더 의미 있는 변수명을 사용해주세요

halfmoon-mind · 2023-09-27T06:45:14Z

+  box-shadow: 0px 4px 6px rgba(0, 0, 0, 0.1);
+`;
+
+interface Props2 {


props1 prop2를 보면 나중에 보기 어려울 것 같아요

halfmoon-mind · 2023-09-27T07:18:07Z

+  const [formData, setFormData] = useState<FormData>({
+    title: f.title,
+    semester: f.semester,
+    studentId: f.studentId,
+    color: f.color,
+    width:f.width
+  });


State범위 너무 큼

halfmoon-mind · 2023-09-27T07:18:53Z

+import { COLORS } from '@/styles/constants/colors';
+import {TEXT_STYLES} from '@/styles/constants/textStyles';
+import styled from '@emotion/styled';
+import React from 'react'; // React import 추가


주석 제거해주세요

halfmoon-mind · 2023-09-27T07:19:17Z

+  width?: number;
+  height?: number;


width height 변수로 받지 말고 상수로 받아주세요
만약 변수로 받는게 필요하다면 옵셔널로 받는 이유가 있나요?

halfmoon-mind · 2023-09-27T07:19:38Z

+
+
+
+interface Props2 {


의미 있는 변수명 필요

halfmoon-mind · 2023-09-27T07:20:25Z

+interface Props2 {
+  fontSize?: number;
+  color?:string;
+  fontFamily?: string;


fontFamily를 받는 이유?

halfmoon-mind

현재 PR에 너무 큰 feature에 1개의 커밋에 올라가니 한 눈에 보기 어렵네요
각 페이지 별로 PR을 분리해서 올리는건 어떤가요?

[feat] 마이페이지 6-2 완료

24a8e85

ChoiSangwon assigned chan0310 Sep 26, 2023

ChoiSangwon requested changes Sep 26, 2023

View reviewed changes

halfmoon-mind requested changes Sep 27, 2023

View reviewed changes

halfmoon-mind added the feat 🔧 새로운 기능 관련된 Issue label Sep 27, 2023

halfmoon-mind linked an issue Sep 27, 2023 that may be closed by this pull request

[feat] 마이페이지 2부분 작성 #94

Open

halfmoon-mind requested review from JooHui-void, SujinKim1127 and jwo0o0 September 27, 2023 07:43

chan0310 marked this pull request as draft September 27, 2023 07:54

halfmoon-mind changed the title ~~[feat] 마이페이지 6-2 완료~~ [#92] 마이페이지 6-2 Sep 27, 2023

[fix]마이페이지

3c1e5cc

This was referenced Oct 10, 2023

[#109] 마이페이지 6-2 분할 첫번째 #130

Open

[#109] 마이페이지 6-2분할 두번째 #131

Open

		width: ${(props) => (props.width ? `${props.width}px` : '18px')};
		height: ${(props) => (props.height ? `${props.height}px` : '18px')};

		@@ -0,0 +1,26 @@
		import { SubmitBox,SubmitBoxButton,SubmitBoxTitle } from "./Header.styles";

	import { SubmitBox,SubmitBoxButton,SubmitBoxTitle } from "./Header.styles";
	import * as styles from "./Header.styles";




		interface Props2 {

Uh oh!

Conversation

chan0310 commented Sep 26, 2023 • edited by HwanGonJang Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Description

Check List

Screenshot

Uh oh!

ChoiSangwon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halfmoon-mind left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halfmoon-mind left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chan0310 commented Sep 26, 2023 •

edited by HwanGonJang

Loading

ChoiSangwon left a comment •

edited

Loading