1조 과제 제출 (조은상, 김필진, 이시우, 정승원) #5
Conversation
trivia: 이메일 feedback, card maxWidth
| const setlist = async () => { | ||
| try { | ||
| const res = await employeeListApi(pageSize); | ||
| const Data = res?.data; |
| setPageSize(Data.response.totalElements + 1); | ||
| } | ||
| if (!Data.success) { | ||
| console.error("등록 실패"); |
There was a problem hiding this comment.
API 통신 결과에 대한 예외처리나 네트워크 예외처리 등이 누락된 부분이 아쉽습니다!
다음번엔 API 통신에 대한 예외처리들을 사용자에게 보여줄 수 있는 팝업이나 메시지로 노출해보시면 좋을 것 같습니다 :)
There was a problem hiding this comment.
프로젝트 제출을 급하게 마무리하게 되어 코드정리 및 예외 처리에 신경을 쓰지 못한 부분들이 많았습니다! 🥹 리팩토링 진행하면서 이 부분을 집중적으로 수정할 예정입니다 감사합니다!
| }; | ||
|
|
||
| // Component | ||
| function AuthForm({ type }: IAuthFormProps) { |
There was a problem hiding this comment.
로그인 폼과 회원가입 폼을 하나의 컴포넌트로 만들어서 type으로 구분하신 것 같은데, 이렇게 구현하신 이유가 따로 있을까요? 이렇게 구현했을 때와 각각의 컴포넌트로 구현했을 때의 차이점이 무엇인지 궁금해서 댓글 남깁니다! 제가 봤을 땐 유지보수 측면에서 더 어려울 거 같다는 느낌이 드는데 어떤 측면에서 좋은 점이 있는지 궁금합니다!
There was a problem hiding this comment.
UI를 공통으로 사용하는 측면에서 바라봤을 때 파일을 나눠서 작업하는 것보다는 type으로 구분하는 것이 더 유리하다고 판단했습니다. 컴포넌트의 UI 다르다면 사실상 나눠서 작업을 하는 것이 유지보수 면에서는 더 좋지만 저희가 UI를 공통적으로 사용하기 때문에 type으로 나눠서 작업을 하게 되었습니다. 다만 제가 이 작업을 했을 때 놓친 부분이라면 오히려 기능적으로 봤을 때 커스텀 hook을 만들어서 작업했으면 어땠을까 하는 아쉬움도 남아있습니다! 좋은 피드백 감사합니다!
| } catch (error) { | ||
| console.error("서버로 부터 응답 없음", error); | ||
| } finally { | ||
| setListUpdate((prev: boolean) => !prev); |
There was a problem hiding this comment.
state를 초기화하는 코드들이 반복적으로 보이는데, 하나의 함수로 묶어서 재사용하면 더 좋을 것 같습니다!
| setPageSize(Data.response.totalElements + 1); | ||
| } | ||
| if (!Data.success) { | ||
| console.log("서버로 부터 응답이 왔는데 에러"); |
There was a problem hiding this comment.
에러 상황에서 예외처리가 없을 경우 사용자에게 아무런 피드백이 없어서 당황스러울 것 같습니다! 예외처리를 한번 적용해보시면 좋을 것 같습니다!
| import { DefaultTheme } from "styled-components"; | ||
|
|
||
| // 추후 색상 변경 | ||
| export const theme: DefaultTheme = { |
There was a problem hiding this comment.
styled-components의 theme를 사용하신 부분이 인상깊습니다! 저도 개인 프로젝트를 하면서 다크모드 적용떄문에 사용해보고 있었는데 한 수 배워갑니다!!!
| @@ -0,0 +1,97 @@ | |||
| import Button from "@components/common/Button"; | |||
| import Link from "next/link"; | |||
There was a problem hiding this comment.
Next.js를 사용하셨네요! 저는 아직까지 React로만 프로젝트를 해봤는데, 코드 보면서 다음 프로젝트에선 Next.js를 도입해보고 싶다는 생각이 들었습니다! 👍
| <h1>연차 결재 현황</h1> | ||
| ) : ( | ||
| <h1>당직 결재 현황</h1> | ||
| )} |
There was a problem hiding this comment.
<h1>${selectedTap} 결재 현황</h1> 이렇게 줄일 수도 있을 것 같아요 :)
There was a problem hiding this comment.
앗 코드 정리를 어떻게 하면 더 직관적인 코드가 될까 고민하고 있었는데 말씀하신데로 하면 더욱 간결하고 읽기 쉬워질것 같습니다! 리팩토링시 반영해보도록 하겠습니다! 😵
| interface selectedTapProps { | ||
| selectedTap: string; | ||
| toggle?: boolean; | ||
| } |
There was a problem hiding this comment.
저는 selectedTap처럼 특정 단어로 제한되는 타입은 selectedTap = "연차" | "당직" | "전체" 이렇게 union type으로 설정하는 편인데 이렇게 하면 IDE에서 intellisense도 동작하고, ts에서 오타도 잡아줘서 좋았습니당
| "axios": "^1.4.0", | ||
| "babel-plugin-styled-components": "^2.1.4", | ||
| "file-saver": "^2.0.5", | ||
| "moment": "^2.29.4", |
There was a problem hiding this comment.
moment는 legacy project인데 이번 프로젝트에서 사용하신 이유가 따로 있으실까용?
There was a problem hiding this comment.
앗 몰랐습니다..!
리액트 캘린더 예시코드에서 moment를 사용해서 그냥 썼습니다
알려주셔서 감사합니다 🙏🏻
| const fetchData = async () => { | ||
| try { | ||
| console.log(year, month); | ||
| const res = await userscheduleApi({ |
There was a problem hiding this comment.
userscheduleApi 안에도 try catch가 있어서 로직의 중복이 있는 것 같습니다!
console.log도 요기 위에 있어요!
fetchData 함수도 hook으로 빼면 좋을 것 같아용
| @@ -0,0 +1,301 @@ | |||
| import React, { useEffect, useState } from "react"; | |||
| import { styled } from "styled-components"; | |||
| import ReactCalendar from "react-calendar"; | |||
| </> | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
리뷰하면서 느꼈는데 styled-component와 children component 구분이 어려워서 로직을 따라기가 힘들더라고요.
styled-component로 개발해보시니까 어떠셨나요?
혹시 styled-component와 children component를 구분 할 수 있는 팁이 있을까요? ㅎㅎㅎㅋㅋ
There was a problem hiding this comment.
비동기처리, 라우팅, 타입별로 나뉘어진것에 대해 가독성 모듈화 캡슐화, 비동기적 UI 업데이트 를 잡으신거 같습니다.
(훅을 나누거나 회원가입, 로그인부분을 나누는 부분은 업무 방식이라 생각합니다)
에러핸들링 부분이 좀 아쉬운것 같습니다.
(저도 제 코드를 쓸때 시간이없어서 에러핸들링을 제대로 하지 못하였습니다)
에러핸들링을 제대로 하면 사용자가 편하게 문제 해결에 도움이 되며, 개발자는 에러메세지가 명확하고 구체적일수록, 어떤 문제가 발생햇는지, 쉽게 파악이 가능합니다. 문제 해결하는데 오히려 드는시간도 단축됩니다 (디버깅)
에러 핸들링
ex)
네트워크 에러 처리 -> 현시점 클라이언트 서버 배포 문제로 오류 시 떳으면 좋았을꺼같습니다!
로그인이 되지않는데 개발자 도구를 열었을시에 로그인 404로 뜨고있습니다.
개발자들은 개발자 도구를 열어서 에러코드로 어떤문제인지 확인 할수있지만 일반사용자는 개발자도구를 열지않습니다. 에러메세지가 명확하고 구체적이면 좋을꺼같습니다!!
const onLogin = useCallback(
async (event: FormEvent) => {
try {
event.preventDefault();
setLoading(true);
const response = await login({ email, password });
if (response?.data?.response) {
localStorage.setItem("Token", response.data.response.accessToken);
localStorage.setItem("empName", response.data.response.empName);
router.push({
pathname: "/employee",
});
} else {
// 처리되지 않은 응답 형식에 대한 에러 핸들링
console.error("Unexpected response format", response);
}
} catch (error) {
// 네트워크 에러 및 서버 응답 에러 핸들링
console.error("Login error:", error);
} finally {
setLoading(false);
}
},
[email, password, router]
);
| { | ||
| key: "1", | ||
| label: ( | ||
| <EmployeeDutyModalForm toggle={toggle} setListUpdate={setListUpdate} /> |
There was a problem hiding this comment.
EmployeeDutyModalForm을 메뉴 아이템 라벨 밖에서 생성 후, 두개의 메뉴 아이템 라벨에서 같은 인스턴스를 참조하도록 하면 재렌더링을 최소화할수 있을것 같아요.
There was a problem hiding this comment.
미니프로젝트 진행하시느라 정말로 고생 많으셨습니다~
이번프로젝트를 통해 협업에 대해서 많은 배움이 있으셨길 바라며 다음 파이널 프로젝트에서도 이번 프로젝트에 배운것들을 더 적극적으로 활용해주시면 좋을꺼같습니다
전체적으로 중복된 코드 와 예외처리, 그리고 공통된 코드 형식을 맞춰주시는게 고민해주시면 좋을꺼같습니다
추가로 질문주셨던 api오류는 제가 완성하신 페이지를 테스트하면서 볼려고 했는데 서버가 다운된건지.. 원인은 잘모르겠으나 api 통신자체가 안되고있어서 확인이 조금 힘든점 양해부탁드리겠습니다..!
| const onLogin = async (event: FormEvent) => { | ||
| event.preventDefault(); | ||
| await login({ email, password })?.then((res) => { | ||
| localStorage.setItem("Token", res.data.response.accessToken); | ||
| router.push({ | ||
| pathname: "/admin", | ||
| }); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
해당 부분에서 async/await 와 then catch 문을 동시에 사용하고 있어서 비효율적인거같습니다
async/await 을 사용하신다면 try/catch문을 통해 예외처리를 진행하시는것을 추천드립니다
또한 프로젝트 내에 react-query가 설치되어있는거 같아서 추후에는 useMutation을 한번 사용해보시는것을 추천드립니다~
There was a problem hiding this comment.
또한 이코드에서 api 성공 후 로그인 실패,성공에 대한 여부가 없어서 무조건 다음페이지로 이동되는 일종의 버그가 있는거같습니다
| const data: IEmployeeMonthly[] = res?.data.response; | ||
| if (selectedTap == "전체") { | ||
| const scheduleData = data; | ||
| setScheduleDatas(scheduleData); | ||
| return; | ||
| } | ||
| if (selectedTap == "연차") { | ||
| const scheduleData = data.filter((item) => { | ||
| if (selectedTap == "연차") { | ||
| return item.orderType == "연차"; | ||
| } | ||
| }); | ||
| setScheduleDatas(scheduleData); | ||
| return; | ||
| } | ||
| if (selectedTap == "당직") { | ||
| const scheduleData = data.filter((item) => { | ||
| if (selectedTap == "당직") { | ||
| return item.orderType == "당직"; | ||
| } | ||
| }); | ||
| setScheduleDatas(scheduleData); | ||
| return; | ||
| } |
There was a problem hiding this comment.
해당 코드를 보시면 중복된 코드가 많이보입니다 예를 들어 아래의 코드는 중복된 코드로 볼수있습니다
const scheduleData = data.filter((item) => {
if (selectedTap == "당직") {
return item.orderType == "당직";
}
});
코드를 해석해보면 특정 orderType을 필터링 하는 코드인데 이를 아래 처럼 간결하게 작성할수있을꺼같습니다
const data: IEmployeeMonthly[] = res?.data.response || [];
if (selectedTap === "전체") {
setScheduleDatas(data);
} else {
const scheduleData = data.filter(item => item.orderType === selectedTap);
setScheduleDatas(scheduleData);
}
| const markStanbyDate = scheduleDatas | ||
| .filter((item) => { | ||
| if (item.status == "대기") { | ||
| return item.status == "대기"; | ||
| } | ||
| }) | ||
| .map((item) => getDateRange(`${item.startDate}`, `${item.endDate}`)); | ||
| //승인 상태 | ||
| const markAprovedDate = scheduleDatas | ||
| .filter((item) => { | ||
| if (item.status == "승인") { | ||
| return item.status == "승인"; | ||
| } | ||
| }) | ||
| .map((item) => getDateRange(`${item.startDate}`, `${item.endDate}`)); | ||
| //반려 상태 | ||
| const markRejectedDate = scheduleDatas | ||
| .filter((item) => { | ||
| if (item.status == "반려") { | ||
| return item.status == "반려"; | ||
| } | ||
| }) | ||
| .map((item) => getDateRange(`${item.startDate}`, `${item.endDate}`)); | ||
|
|
||
| // 달력에 mark 될 날짜 합쳐서 새로운 배열 생성 (대기/승인/반려) | ||
| const stanByDate: string[] = ([] as string[]).concat(...markStanbyDate); | ||
| const aprovedDate: string[] = ([] as string[]).concat(...markAprovedDate); | ||
| const rejectedDate: string[] = ([] as string[]).concat(...markRejectedDate); |
There was a problem hiding this comment.
마찬가지로 이 코드도 중복된 부분이 많이보입니다 status 별로 데이터는 재가공하는 코드인거같은데 이부분을 함수로 만들어서 재사용하면 좋을꺼같습니다 또한 이렇게 함수로 만들고 useMemo 혹은 useCallback으로 묶어 버리면 성능에 더 유리할꺼같습니다~
const filterAndMapDates = (statusValue: string) => {
return scheduleDatas
.filter(item => item.status === statusValue)
.map(item => getDateRange(`${item.startDate}`, `${item.endDate}`))
.flat();
};
| const columns: ColumnsType<IDataSourceItem> = [ | ||
| { | ||
| title: "사원명", | ||
| dataIndex: "사원명", | ||
| key: "사원명", | ||
| align: "center", | ||
| render: (_, data) => ( | ||
| <> | ||
| <p>{data.empName}</p> | ||
| </> | ||
| ), | ||
| }, | ||
| { | ||
| title: "결재요청날짜", | ||
| dataIndex: "결재요청날짜", | ||
| key: "결재요청날짜", | ||
| align: "center", | ||
| render: (_, data) => ( | ||
| <> | ||
| <p>{data.createdAt}</p> | ||
| </> | ||
| ), | ||
| }, | ||
| { | ||
| title: "유형", | ||
| dataIndex: "유형", | ||
| key: "유형", | ||
| align: "center", | ||
| render: (_, data) => ( | ||
| <> | ||
| <p>{data.orderType}</p> | ||
| </> | ||
| ), | ||
| }, | ||
| { | ||
| title: "승인여부", | ||
| dataIndex: "승인여부", | ||
| key: "승인여부", | ||
| align: "center", | ||
| render: (_, data) => { | ||
| if (data.status === "대기") { | ||
| return ( | ||
| <Button | ||
| pending="true" | ||
| onClick={() => { | ||
| type === "admin" | ||
| ? adminOnClickHandler(data) | ||
| : employeeOnClickHandler(data); | ||
| }} | ||
| > | ||
| {data?.status} | ||
| </Button> | ||
| ); | ||
| } else if (data.status === "승인") { | ||
| return ( | ||
| <Button | ||
| accept="true" | ||
| onClick={() => { | ||
| type === "admin" | ||
| ? adminOnClickHandler(data) | ||
| : employeeOnClickHandler(data); | ||
| }} | ||
| > | ||
| {data?.status} | ||
| </Button> | ||
| ); | ||
| } else { | ||
| return ( | ||
| <Button | ||
| deny="true" | ||
| onClick={() => { | ||
| type === "admin" | ||
| ? adminOnClickHandler(data) | ||
| : employeeOnClickHandler(data); | ||
| }} | ||
| > | ||
| {data?.status} | ||
| </Button> | ||
| ); | ||
| } | ||
| }, | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
코드에 중복된 부분이 많습니다 변수가 조금 커서 코드를 못드리는점 죄송하며 아래 내용 참고부탁드리겠습니다~
- buttonProps 변수를 만들고 status 따라 buttonProps에 값은 변경하면 return 하나로 해결될꺼같습니다
- 불필요한 플래그먼트 코드가 많습니다 단순히
태그만 반환한다면 플래그먼트는 불필요해 보입니다
| interface selectedTapProps { | ||
| selectedTap: string; | ||
| toggle?: boolean; | ||
| } |
There was a problem hiding this comment.
전체적으로 interface 명명 규칙을 확인해보니 어떤분은 'I'를 붙이는 분도 계시고 개발자 분들마다 규칙이 많이 다른거같습니다
해당 프로젝트는 많은 개발자가 하나의 프로젝트를 같이 작업하는거기때문에 이러한 명명 규칙을 정확하게 지정해주시는게 좋습니다
| {selectedTap == "전체" ? ( | ||
| <ItemContainer> | ||
| <ul> | ||
| {datas && | ||
| datas.map((data) => { | ||
| return ( | ||
| <Employeedata | ||
| key={data.id} | ||
| onClick={() => { | ||
| openHandler(data); | ||
| }} | ||
| > | ||
| <Space | ||
| direction="horizontal" | ||
| size="middle" | ||
| style={{ width: "200px" }} | ||
| > | ||
| {data.status === "대기" ? ( | ||
| <StanByIcon /> | ||
| ) : data.status === "반려" ? ( | ||
| <RejectIcon /> | ||
| ) : data.orderType === "연차" ? ( | ||
| <AnnualIcon /> | ||
| ) : ( | ||
| <DutyIcon /> | ||
| )} | ||
| <DutyInfo>{data.startDate}</DutyInfo> | ||
| <DutyInfo> | ||
| {data.status == "대기" | ||
| ? `승인 ${data.status}` | ||
| : `${data.status} 완료`} | ||
| </DutyInfo> | ||
| </Space> | ||
| </Employeedata> | ||
| ); | ||
| })} | ||
| </ul> | ||
| </ItemContainer> | ||
| ) : ( | ||
| <ul> | ||
| {datas && | ||
| datas | ||
| .filter((data) => { | ||
| if (selectedTap == "연차") { | ||
| return data.orderType == "연차"; | ||
| } | ||
| if (selectedTap == "당직") { | ||
| return data.orderType == "당직"; | ||
| } | ||
| }) | ||
| .map((data) => { | ||
| return ( | ||
| <Employeedata | ||
| key={data.id} | ||
| onClick={() => { | ||
| openHandler(data); | ||
| }} | ||
| > | ||
| <Space | ||
| direction="horizontal" | ||
| size="middle" | ||
| style={{ width: "200px" }} | ||
| > | ||
| {data.status === "대기" ? ( | ||
| <StanByIcon /> | ||
| ) : data.status === "반려" ? ( | ||
| <RejectIcon /> | ||
| ) : data.orderType === "연차" ? ( | ||
| <AnnualIcon /> | ||
| ) : ( | ||
| <DutyIcon /> | ||
| )} | ||
| <DutyInfo className="state">{data.startDate}</DutyInfo> | ||
| {data.status == "대기" | ||
| ? `승인 ${data.status}` | ||
| : `${data.status} 완료`} | ||
| </Space> | ||
| </Employeedata> | ||
| ); | ||
| })} | ||
| </ul> | ||
| )} |
There was a problem hiding this comment.
selectedTap 에 값에 따라 조건부 렌더링을 진행하고 있습니다, 이 코드를 자세히보시면 중복된 코드가 몇몇군데 보입니다
이부분도 코드가 길어서 개선된 코드를 전달드리지는 못할꺼같고 텍스트로 전달드리는점 양해부탁드립니다
- datas 를 순회하는 코드가 중복됩니다 물론 filter 하는 부분이 다른지만 기본적으로 데이터의 형식은 동일해보입니다 따라서 아래처럼 변수 하나는 선언하면 코드를 줄일수있을꺼같습니다
const filterData = selectedTap === '전체' ? datas : datas.filter(data => data.orderType == selectedTap);
- 현재 data.status 에 따른 아이콘 렌더링 코드가 중복되어보입니다 이부분을 함수로 빼주시면 추후 관리가 더편리해지고 이 함수를 최적화 코드로 만든다면 더욱 성능이 좋을꺼같습니다
- 값을 비교할때는 '==' 아닌 '===' 을 써주시면 좋습니다
- 또한 지금까지의 코드들을 보니 '전체','대기' 등 상수 값들이 많아 보입니다 이러한 상수는 별도의 파일로 관리하시는것을 추천드립니다
| } else if (key === "전체") { | ||
| } |
| @@ -0,0 +1,35 @@ | |||
| import axios, { AxiosRequestConfig } from "axios"; | |||
|
|
|||
| const BASE_URL = "http://3.34.110.127"; | |||
There was a problem hiding this comment.
현재는 크게 상관이 없지만 실제 현업에서는 prod,stg,dev 각 빌드환경이 나눠져있어서 이러한 baseUrl은 환경변수로 관리해주시는게 좋습니다~
WORK FAIRY(근태 요정)
👩🚀 프론트엔드 개발팀
👑 조은상
김필진
이시우
정승원
- 프론트엔드 팀장
- README 및 프로젝트 관련 서류 담당
- 프로젝트 리더
- Git Management
- 와이어프레임 담당
- 템플릿 디자인 담당
사용기술 및 개발환경
Development
화면 구성
로그인 페이지
사용자별 로그인
회원가입
사원용 로그인
관리자용 로그인
사원 페이지
메인페이지
연차/당직 등록
결재 내역 페이지
결재 상세 내역
관리자 페이지
요청 관리 페이지
결재 처리 창
일별 사용대장
월별 사용대장
사원 조회 페이지
현재 리팩토링 중!