-
Notifications
You must be signed in to change notification settings - Fork 0
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
9조 과제 제출 (김성은, 안태욱, 이용수, 최용준) #10
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요! 미니프로젝트 8조 이은지입니다
제가 맡았던 부분인 메인캘린더 부분만 코멘트 남겨두었습니다
fullcalendar 라이브러리 동일하게 사용하셔서 너무 반가웠고, 이벤트 필터링하는 부분에서 고민많이하셨을거 같아요 (제가 그랬습니다..ㅎㅎ)
fullcalendar라고 느껴지지 않을만큼 디자인도 엄청 공들여서 하신부분이 너무 좋았어요!! 저는 하나두.. 안해서 반성중입니다..
프로젝트 진행하시느라 고생많으셨습니다😊
} | ||
}; | ||
fetchMainInfo(); | ||
}, []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useEffect의 두번째 인자를 빈 배열로 추가하신 이유가 있을까요? 처음 마운트될때만 실행되는것을 의도하셨을지 궁금합니다! 만약 그게 아니라면 빈배열 대신 이벤트 필터링을 하는 filteredEvents
, 스케줄 정보를 가공한 processedEvents
를 추가한다면 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
은지님 제안 감사합니다 !
솔직히 타입 에러 잡으려고 의존성 배열에 무관심 했습니다 ㅠㅠㅠㅠㅠ
리팩토링 할 때 한 번 추가해 볼게요 !
// 당직 리스트 개수 | ||
const selectedDuty = events.filter( | ||
(event: any) => event.category === "당직", | ||
).length; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
selectedAnnualLeave
, selectedDuty
가 같은로직을 사용하고 있는데, 함수화 시켜서 코드중복을 줄여보면 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
맞습니다 !!
시간에 쫓겨서 로직 짤 생각만 하고 가독성 있는, 효율 좋은 코드를 짤 생각은 못 했네여 ㅠㅠ
const mainInfo = await getMainPage(ACCESSTOKEN ?? ""); | ||
|
||
if (mainInfo?.data.annuals && Array.isArray(mainInfo.data.annuals)) { | ||
const processedEvents = mainInfo.data.annuals.map((annuals: any) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
map 함수의 파라미터로 annuals
대신 annual
이라고 명시하는게 어떨까요?
map함수의 파라미터로 복수 보다는 단수 아이템이라는것이 직관적으로 보일거 같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제안 감사합니다 !!
제가 받아온 api 응답 데이터 배열의 이름이 annuals 속성 안에 있어서 annuals로 지정했습니다 !
annuals를 annual로 선언해서 명시적으로 사용하라는 말씀일까요 ?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네네 맞습니다!!
left: "prev", | ||
center: "title", | ||
right: "next", | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 headerToolbar를 커스터마이징 하고싶었는데 되게 간단하군요! 이번 리팩토링에 적용해보겠습니다! 감사해요~
type="checkbox" | ||
className="input_checkbox" | ||
checked={isAllEventsChecked} | ||
onChange={handleAllEventsToggle} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전체 일정 체크박스를 해제하면 자신의 일정만 떠야한다고 이해했는데 맞나요?
지금 캘린더에서는 내일정만 따로 보이지 않아서 궁금합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
맞습니다ㅠㅠ
시간이 없어서 흐지부지된 컴포넌트입니다 ㅠㅠ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
9조 모든 분들 미니 프로젝트 고생 많으셨습니다! 신청 및 신청 리스트에서 궁금한 점 코멘트 달아 놓았습니다!
toggle: (param: boolean) => void; | ||
} | ||
|
||
export default function Modal(props: PropsWithChildren<ModalProps>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ReactNode 대신 PropsWithChildren을 쓰는 방법도 있군요! 다만 children타입이 옵셔널이 아닌경우엔 제네릭으로 children의 타입 범위를 좁혀주는것도 좋다고 하네요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
맞습니다 !! 좋은 의견 감사합니다ㅎㅎㅎㅎ
const response = await postMain( | ||
newEvent.title, | ||
newEvent.category, | ||
newEvent.endDate, | ||
newEvent.reason, | ||
newEvent.startDate, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newEvent라는 객체를 통째로 파라미터로 넘겨주는 방식으로 설계가 됐으면 더 편했을 것 같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
맞습니다 !! 하나 배워갑니다 !
<option value="기타휴가">============= 당직 =============</option> | ||
)} | ||
{newEvent.category === "연차" && ( | ||
<> | ||
<option value="">========== 선택하세요 ===========</option> | ||
<option value="연차유급휴가">연차 유급 휴가</option> | ||
<option value="병가휴가">병가 휴가</option> | ||
<option value="경조사휴가">경조사 휴가</option> | ||
<option value="출산휴가">출산 휴가</option> | ||
<option value="기타휴가">기타 휴가</option> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
option 태그의 value값은 기본적으로 innerHTML값을 가져오도록 되어있습니다. 띄어쓰기 유무때문에 일부러 value를 기입해두신 걸까요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
동민님 말씀대로 value 값을 명시하지 않아도 innerHTML값을 가지고 오는 걸로 알고 있습니다.
백엔드 분들에게 요청한 api 명세서에 값을 띄어쓰기로 명시해서 제출했는데,
후에 확인해보니 띄어쓰기 없이 값이 온 것을 확인했습니다.
그 때는 이미 시간도 많이 부족하고 백엔드, 프론트엔드 분들도 다들 열심히 하고 계셔서
그냥 제 선에서 타협하기로 하고 value 값을 명시했습니다 !
다국어 사이트라고 가정했을 때
유지 보수 쪽에서 장점을 가지고 오지 않을까
좋게 생각하고 있습니다 !
질문 감사합니다 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
유지 보수를 언급하셔서 짧게 첨언하자면, 일반적으로 이런 도메인 내에서 자주 반복되는 변수들은 Enum 등으로 정의해두고 해당 값을 주고 받도록 설계를 하는 경우도 많습니다.
예)
0: 연차유급휴가
1: 병가휴가
2: 경조사휴가
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 8조 문대현입니다
제가 맡았던 관리자 페이지 부분 코멘트 남겨두었습니다 !
9조분들 프로젝트 고생 많으셨습니다
const handlePermissionClick = async (): Promise<void> => { | ||
if (status === "결재 대기") { | ||
try { | ||
const res: AxiosResponse = await changeAdminData.mutateAsync(item); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
결재 버튼을 클릭했을 때 페이지의 버튼이 새로고침을 해야 결재 완료로 바뀌는데
새로고침하지 않고 버튼을 클릭했을 때 바로 리렌더링이 되면 더 좋을 것 같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리랜더링으로 하고 싶었는데 잘안되었네요! 리팩토링 하면서 수정해보겠습니다! :) 감사합니다!
{calculateUsedDays()}일 | ||
</span> | ||
</span> | ||
<button |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리렌더링이 바로 되고 있지 않고 있기 때문에 버튼을 클릭했을 때
alert이라던가 클릭했을 때 변화가 일어났음을 알려주는 게 있으면 좋을 것 같습니다
현재는 결재 대기 버튼을 눌러도 아무런 변화가 없기 때문에 눌러도 제대로 작동하고 있는지
새로고침을 해야 알 수 있더라고요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alert 방법 사용해보겠습니다! :) 좋은 의견 감사합니다!
const handlePermissionClick = async () => { | ||
if (status === "결재 대기") { | ||
try { | ||
const response = await permission(item); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handlePermissionClick함수를DayoffLists.tsx DutyLists.tsx 두 파일에서 사용중인데
공통 함수로 분리하고 두 컴포넌트에서 재사용하는 방식으로 리팩토링하는 방법도 있을 것 같습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋은 의견 감사합니다! 리팩토링 시 참고해서 진행해 보겠습니다!
/^(([^<>()\\[\].,;:\s@"]+(\.[^<>()\\[\].,;:\s@"]+)*)|(".+"))@(([^<>()[\].,;:\s@"]+\.)+[^<>()[\].,;:\s@"]{2,})$/i; | ||
const passwordRegex = // 영문, 숫자, 특수문자 포함 8자 이상 | ||
/^(?=.*[a-zA-z])(?=.*[0-9])(?=.*[$`~!@$!%*#^?&\\(\\)\-_=+])(?!.*[^a-zA-z0-9$`~!@$!%*#^?&\\(\\)\-_=+]).{8,20}$/; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요. 코드 리뷰를 맡은 8조 정재현입니다. 제가 리뷰 담당한 부분은 회원가입, 로그인, 인증처리, 마이페이지입니다!
상태 변수가 많이 사용되었는데, 객체를 사용하여 관련된 상태들을 그룹화 하는 것은 어떨까요?
예를 들어, 입력 값을 관리하는 상태는 하나의 객체로 그룹화 할 수 있듯이요!
const [userInfo, setUserInfo] = useState({
email: '',
password: '',
confirmPassword: '',
name: '',
});
const handleLogout = async (): Promise<void> => { | ||
try { | ||
navigate("/"); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
로그아웃 API를 호출하기 전에, navigate("/")를 사용해 메인 페이지로 먼저 리다이렉트하고 있습니다. 혹시 이유가 있으실까요?
console.error("로그아웃이 실패 하였습니다.", error); | ||
} | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
로그아웃이 실패한 경우에도, 콘솔 뿐 아니라 사용자에게 alert 등으로 알려주면 좋을 것 같습니다!
) : ( | ||
<span onClick={handleEdit}>{dutyItem?.startDate}</span> | ||
)} | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
당직의 경우, 메인 화면에서 하루가 아니라 여러일을 한 번에 신청 가능하게 되어 있는데(예를 들어 8/15~8/17, 이렇게 3일간), 마이페이지 당직 리스트에서 수정할 경우, 시작일만 수정 가능하게 하신 이유가 있나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네 당직일은 저희가 생가하기에 하루에 이뤄진다 생각을 햇습니다. 그래서 당직의 경우 시작일과 종료일이 같게 되어있습니다. 그래서 시작일만 수정하면 당직 전체 일정이 변경된다 보시면 될것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이틀 이상의 당직을 경험해보지 못하셨다니...부럽네요
{ | ||
headers: { | ||
Authorization: `Bearer ${ACCESSTOKEN}`, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
headers: { Authorization: Bearer ${ACCESSTOKEN}, }와 같은 코드가 여러 곳에서 반복되고 있습니다. Axios 인터셉터를 사용해 보시면 어떨까요? 코드중복을 피할 수 있어서 좋은 것 같습니다. 저희 팀에서도 이번에 사용했습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
백업 파일을 생성해두신 이유가 있나요?
파일 내용만 보면 이 폴더에 없었을 파일 같은데, 어쩌다 여기 생성된 건지 궁금하네요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 파일 직접 작성하신 건가요? 아니면 트랜스파일된 결과가 src에 남은 건가요?
}; | ||
|
||
|
||
const ACCESSTOKEN = getAccessToken(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이러면 access token이 만료된 경우는 어떻게 대응하나요?
이 부분 코드만 보면 access token을 전역 상수로 선언해두신 느낌이네요.
async (error) => { | ||
if (error.response.status === 403) { | ||
try { | ||
const NEW_ACCESSTOKEN = await getNewAccessToken(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이런 형태의 변수 네이밍은 보통 파일 최상단에 있는 전역 변수에만 사용합니다!
console.log("재요청에러: ", error); | ||
} | ||
} | ||
throw error; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
403으로 응답되지 않았을 땐 throw만 하고 있네요.
아래 api fetcher들에 반복되는 try / catch를 줄이기 위해, 조금 더 공통적으로 에러를 처리할 수 있는 방안을 고민해보셔도 좋을 것 같습니다.
추가로 이런 케이스엔, if (error.response.status !== 403)
일 때 throw
하는 early exit 패턴을 사용하면, 코드를 조금 더 간결하게 유지할 수 있습니다.
const startTime = new Date(annuals.startDate); // 시작 시간을 Date 객체로 변환 | ||
const endTime = new Date(annuals.endDate); // 종료 시간을 Date 객체로 변환. | ||
|
||
if (startTime.toISOString() === endTime.toISOString()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getTime()
으로 비교하셔도 되지 않을까요?
setProcessedEvents(processedEvents); | ||
setUserName(mainInfo.data.username); | ||
const ROLE = localStorage.getItem("role"); | ||
setRole(ROLE ?? null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ROLE
이 nullish할 때 null
이 설정되고 있는데, 의도된 동작일까요?
const today = new Date(); | ||
const year = today.getFullYear(); // 년도 (예: 2023) | ||
const month = today.getMonth() + 1; // 월 (0 ~ 11, 1을 더해서 1 ~ 12로변환) | ||
const day = today.getDate(); // 오늘 날짜 (1 ~ 31) | ||
const daysOfWeek = ["Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat"]; | ||
const dayOfWeek = daysOfWeek[today.getDay()]; // 요일 (0 ~ 6) | ||
const formattedDate = `${year}. ${month}. ${day}. ${dayOfWeek}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Date
를 받아서 string
을 반환해주는 유틸로 만들 수 있지 않을까요?
handleAddEvent={function (): void { | ||
throw new Error("Function not implemented."); | ||
}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
런타임 에러 발생하고있을 것 같네요
category={category} | ||
setCategory={setCategory} | ||
/> | ||
{!category ? ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
category ? truthy : falsy
로 작성하는 편이 조금 더 읽기 편하지 않을까요?
KDT5-Mini project
'TimeSync'
회원가입과 로그인을 통해 인증된 사용자가 각자 자기의 연차 휴가 관리를 할 수 있으며,
관리자의 승인 기능으로 결재 유무를 확인 하게 작성 되었습니다.
메인 캘린더를 활용하여 각자의 연차와 당직의 스케쥴을 확인 할 수 있으며,
캘린더 데이터를 기초로 스케쥴을 조정 할 수 있습니다.
• 배포 주소: 타임싱크 배포 링크
관리자 계정 : [email protected] / fastcampus12#$
혹시 로그인 페이지가 보이지 않으시면 로컬 스토리지 내역을 삭제하시고 새로고침 해주세요
• 프로젝트 노션: 9조 노션
🗓 프로젝트 기간: 2023.07.24 ~ 2023.08.11
🧔 개발팀
로그인,
회원가입,
admin 인증
연차/당직 등록기능,
메인캘린더 리스트 작업
Admin 페이지,
검색기능 구현,
Admin 연차/당직 승인 기능 구현
연차/당직 수정,
삭제 기능
기술 스택
Development
Config
Deployment
Environment
Cowork Tools
📌 프로젝트 기능
프로젝트 스크린샷 화면
🙏 감사합니다