🔥🔥🔥 코드리뷰 PR 🔥🔥🔥 by ssumanlife · Pull Request #77 · Dev-FE-1/Toy_Project_II_team2

ssumanlife · 2024-08-09T03:43:13Z

코드리뷰용 PR입니다!

메인과 개발 브랜치 동기화

[Feature] 임시 라우팅 기능 구현 및 헤더 스타일링

[Feature] Emotion으로 글로벌 스타일 설정

Feat/firebese db test 9

[feature] 버튼, 모달창 컴포넌트 작성

[Feature] 사장님 급여내역 페이지 퍼블리싱 완료

:wq#

[fix] 캘린더, 직원 홈 디데이컴포넌트 위치 css 수정

iamidlek

고생하셨습니다.
실제 배포된 사이트는 있으나 로그인 예시 값이 없어 확인이 어렵습니다
혹은 로컬에서 확인 할 수 있게 환경 변수를 공유주시면
리뷰할 때 더 자세히 볼 수 있을 것 같습니다.
반복된 부분은 코멘트 생략하였습니다.

폴더 경로 대소문자 문제로 코드 리뷰에 어려움이 많이 있었습니다.
https://k1005.github.io/2021/07/31/naming-rules/#google_vignette

git commit 메세지도 일부 컨벤션이 잘 지켜지지 않는 것 같습니다.

불필요한 파일이 PR에 포함되어있습니다. store.tsx

컨벤션 없이 각자의 코드 스타일로 작성되었습니다.

기본 환경 설정이 (eslint 등) 제대로 동작하지 않는 것 같습니다.

import App from './App.tsx'; 와 같이 .tsx 가 생략되면 import 되지 않는 문제가 있습니다.

캘린더 등 매우 복잡하게 작성된 컴포넌트나
불필요한 상태나 useEffect 등이 과하게 많이 작성된 경우가 있는 것 같습니다.

iamidlek · 2024-08-11T17:06:28Z

+    "@fullcalendar/react": "^6.1.15",
+    "@mui/material": "^5.16.6",
+    "@reduxjs/toolkit": "^2.2.7",
+    "@types/react-router-dom": "^5.3.3",


devDependencies에 있어야 할 것 같습니다.

iamidlek · 2024-08-11T17:07:01Z

+    "shortid": "^2.2.16",
+    "style-loader": "^4.0.0",
+    "uuid": "^10.0.0",
+    "vite": "^5.3.4"


동일하게 devDependencies에 있어야 할 것 같습니다.
개발에 필요한 것과 실제 배포에 필요한 내용이 구분되면 좋을 것 같습니다.

iamidlek · 2024-08-11T17:12:40Z

+{
+  "presets": ["@babel/preset-env", "@babel/preset-react", "@babel/preset-typescript"],
+  "plugins": ["@emotion"]
+}


필요한 설정인지 궁금합니다.

iamidlek · 2024-08-11T17:14:54Z

+import { configureStore, ThunkAction, Action } from '@reduxjs/toolkit';
+import modalSlice from './Reducers/ModalSlice.ts';
+import employeeSalarySlice from './Reducers/EmployeeSalarySlice.tsx';
+import calendarSlice from './Reducers/CalendarEventSlice.ts';
+
+const store = configureStore({
+  reducer: {
+    modal: modalSlice,
+    employeeSalary: employeeSalarySlice,
+    calendar: calendarSlice,
+  },
+});
+
+export type RootState = ReturnType<typeof store.getState>;
+export type AppDispatch = typeof store.dispatch;
+export type AppThunk<ReturnType = void> = ThunkAction<ReturnType, RootState, unknown, Action<string>>;
+
+export default store;


tsx 확장자가 아니며 ts 파일이 있으므로 지워져야할 파일일것 같습니다.

store.tsx 삭제하고 tsx가 아닌 파일들은 다시 ts파일로 수정했습니다!

iamidlek · 2024-08-11T17:24:10Z

+import { RouterProvider, createBrowserRouter } from 'react-router-dom';
+import store from './store.tsx';
+import App from './App.tsx';
+import Error from './Pages/Error.tsx';


pages폴더는 소문자인데 import 경로는 대문자로 되어있어
window에서 에러가 발생합니다.

iamidlek · 2024-08-11T18:12:30Z

+    const memberPromises = membersSnapshot.docs.map(async (memberDoc) => {
+      const collectionSnapshot = await getDocs(collection(db, `members/${memberDoc.id}/payrollCorApp`));
+
+      const payDataPromises = collectionSnapshot.docs.map(async (payDataDoc) => {
+        const payDataId = payDataDoc.id;
+        const payData = payDataDoc.data();
+
+        if (payData.name === name && payData.month === month && payData.correctionDetails === correctionDetails) {
+          await setDoc(doc(db, `members/${memberDoc.id}/payrollCorApp`, payDataId), {
+            name: payData.name,
+            month: payData.month,
+            correctionState: state,
+            correctionDetails: payData.correctionDetails,
+            reasonForApplication: payData.reasonForApplication,
+          });
+        }
+      });
+
+      await Promise.all(payDataPromises);
+    });
+
+    await Promise.all(memberPromises);


중첩된 map 내에서 여러 요청을 날릴 수 밖에 없는 구조인가요?

iamidlek · 2024-08-11T18:15:00Z

+import { css } from '@emotion/react';
+import Select from '../Select'; 
+
+const customSelectStyles = css`


이모션 css 내용이 파일마다
컴포넌트 상위, 내부, 하위에 각각 적혀있어
맞춰주시면 좋을 것 같습니다.

iamidlek · 2024-08-11T18:16:34Z

+  // 총 지급액
+  const sumPay = (payData.baseSalary + payData.weeklyHolidayAllowance + payData.additionalAllowance)
+    .toFixed(0)
+    .replace(/\B(?=(\d{3})+(?!\d))/g, ',');


어떤 처리인지 변수명으로 관리되면 좋을 것 같습니다.

변수명 수정했습니다!!

iamidlek · 2024-08-11T18:16:50Z

+    .toFixed(0)
+    .replace(/\B(?=(\d{3})+(?!\d))/g, ',');


처리가 반복되고 있어 함수로 빼도 좋을 것 같습니다.

반복되는 처리는 함수로 변경했습니다!

iamidlek · 2024-08-11T18:17:32Z

+  // 그 외 나머지 pay
+  const payArr = [];
+  for (let i = 0; i < Object.values(payData).length; i++) {
+    payArr.push(


for of 혹은 for in 사용을 추천드립니다.

map을 사용하여 수정했습니다!

[Refactor] 코드수정 및 ts파일로 복구

[Refector] 급여수정시 수정되지않으면 버튼이 유지되도록 수정

…into fix/employee-87

[Refactor] 직원리스트 페이지 리팩토링

[Refactor] 일정관리페이지 리펙토링

…into refactor/code-refactoring-89

[리팩토링] 코드 리팩토링

[Refactor] 중복 폴더 삭제

dev 브랜치를 main 브랜치로 머지

James and others added 30 commits July 25, 2024 09:49

Initial Project Setting

f5f5429

ESLint & Prettier Setting

1b09e0d

enables TS image import

0004123

PR & Issue template

61a2361

PR 템플릿 위치 변경

1bc907d

필요한 라이브러리 설치 및 폴더 구조 변경

465f6d1

Merge pull request #2 from Dev-FE-1/dev

9266efc

메인과 개발 브랜치 동기화

feat: 임시 페이지 구현 (#5)

b99ec28

style: 필요한 아이콘 및 사진 추가 (#5)

697f39b

chore: 패키지 매니저 수정 (#5)

031df39

feat: 임시 라우팅 기능 구현 (#5)

0e99b0d

feat: firebase 코드와 유저 관련 Context 코드 추가 (#5)

cb50f37

Merge pull request #8 from Dev-FE-1/feat/routing-func-5

c8810ae

[Feature] 임시 라우팅 기능 구현 및 헤더 스타일링

feat: 전역 변수 설정 (#7)

04822f3

feat: Global style 설정 (#7)

c20792f

Merge pull request #10 from Dev-FE-1/feat/emotion-setting-7

1b81b2b

[Feature] Emotion으로 글로벌 스타일 설정

1차시도실페

9f63e79

firebase넣기,읽기customHook

0518214

Merge pull request #12 from Dev-FE-1/feat/firebese-db-test-9

89a56b5

Feat/firebese db test 9

fix:npm run오류수정

2b372cc

급여정정페이지퍼블리싱1차

563a190

퍼블리싱작업1

2ded072

feat:사장님급여내역페이지퍼블리싱

0b13f2b

feat: 직원페이지 스타일링 작성중 (#6)

c61048f

feat: 버튼,모달 컴포넌트 작성 (#13)

c8c014d

feat/사장님급여내역페이지퍼블리싱완료

6efd0b8

Merge pull request #15 from Dev-FE-1/feat/buttonModal-components-13

2130ebc

[feature] 버튼, 모달창 컴포넌트 작성

Merge pull request #16 from Dev-FE-1/feat/salary-page-3

07579d9

[Feature] 사장님 급여내역 페이지 퍼블리싱 완료

feat: 캘린더 기본형태 #4

80f22b0

:wq#

feat:캘린더 퍼블리싱(#4)

1ac1e5b

Merge pull request #85 from Dev-FE-1/feat/mediaQuery-76

37c8bdb

[fix] 캘린더, 직원 홈 디데이컴포넌트 위치 css 수정

iamidlek force-pushed the codeReview branch from 4f3b0f8 to 0b2cc83 Compare August 11, 2024 17:03

iamidlek reviewed Aug 11, 2024

View reviewed changes

ssumanlife and others added 27 commits August 13, 2024 14:44

refactor: 코드수정 및 ts파일로 복구

6530613

Merge pull request #86 from Dev-FE-1/refactor/payrollHistory-78

f6cf3b9

[Refactor] 코드수정 및 ts파일로 복구

refector:급여수정시 수정되지않도록수정

e95b36c

fix: 직원추가 연락처, 은행 데이터 수정

52eb4ba

refactor: firebaseAPI코드수정

552d13b

Merge pull request #88 from Dev-FE-1/refactor/payrollHistory-78

a2558e6

[Refector] 급여수정시 수정되지않으면 버튼이 유지되도록 수정

rename: 폴더명 변경 (#89)

465c688

rename: 파일명 변경에 따른 import 수정 (#89)

7c6b78e

refactor:캘린더 css및 기능리펙토링(#90)

6f68dde

refactor: firebase/api 쿼리 사용(#90)

77675df

refactor: console.error -> console.warn 수정 (#90)

05aa738

fix: 직원추가 토스트창 구현 #87

fea4783

fix: 유효성검사 추가 #87

db06553

Merge branch 'dev' of https://github.com/Dev-FE-1/Toy_Project_II_team2 …

8707e0c

…into fix/employee-87

feat:Promise중첩 수정

3a000fc

fix: console.error warn으로 변경 #87

2cf11c0

Merge pull request #92 from Dev-FE-1/fix/employee-87

b022fe7

[Refactor] 직원리스트 페이지 리팩토링

Merge pull request #91 from Dev-FE-1/refactor/code-refactoring-90

c37ae5c

[Refactor] 일정관리페이지 리펙토링

feat: 로딩애니메이션 컴포넌트 추가 (#89)

cb1540f

Merge branch 'dev' of https://github.com/Dev-FE-1/Toy_Project_II_team2 …

ad08956

…into refactor/code-refactoring-89

refactor: 코드 리팩토링 (#89)

8f0eaf5

refactor: 코드 리팩토링 (#89)

a6f2bac

Merge pull request #93 from Dev-FE-1/refactor/code-refactoring-89

fa72fc4

[리팩토링] 코드 리팩토링

캐시된 파일들 삭제

787c90a

fix: 월급날 업데이트가 모든 직원에게 적용되도록 변경 (#94)

4e90c3c

Merge pull request #95 from Dev-FE-1/bug/folder-rename-94

6761042

[Refactor] 중복 폴더 삭제

Merge pull request #96 from Dev-FE-1/dev

cadf201

dev 브랜치를 main 브랜치로 머지

Conversation

ssumanlife commented Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iamidlek 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ssumanlife commented Aug 9, 2024 •

edited

Loading