FE 5기_2조 과제 제출 (김경원, 황인승, 윤금엽) #1
Conversation
| // 일반 유저 | ||
| return <Outlet /> | ||
| } | ||
| alert('로그인이 필요합니다.') |
There was a problem hiding this comment.
ux적인 관점에서 alert()는 불필요해 보입니다.
사용자는 해당 프로그램이 로그인을 필요로 하는 것을 알고 있습니다.
그리고 alert, prompt, confirm은 사용이 권장되지 않습니다.
There was a problem hiding this comment.
꼼꼼한 리뷰 감사합니다! 인수분해 해주셔서 많은 반성과 배움을 얻을 수 있었습니다. 점검해보니 관리자 부분은 백엔드 측과 이야기가 필요할 것 같네요. 테스트할 때는 문제가 없었는데.. 빠른 시간 안에 수정 후 다시 말씀 드리겠습니다.🥲
| import axios from 'axios' | ||
|
|
||
| const baseApi = axios.create({ | ||
| baseURL: import.meta.env.VITE_API_URL, |
There was a problem hiding this comment.
api end point를 숨기는 것이 보안적인 측면에서 좋다는 것을 알았습니다.👍
그리고 저희조도 .env 숨기는 작업 하지 않았지만🤣🤣 네트워크 탭에서 주소가 다 노출이 되므로 서버리스 함수가 필요합니다.
There was a problem hiding this comment.
서버리스도 고민을 했지만 이미 깃허브에 다 올라가버려서..😂 시간 상의 문제도 있었고 해서 고려하지 않았습니다. 리뷰 후 수정 진행하면서 시도해봐야겠네요!
|
|
||
| const [loginButtonDisabled, setLoginButtonDisabled] = useState<boolean[]>([false, false, false]) | ||
|
|
||
| const handleSignUp = async (e: React.FormEvent<HTMLFormElement> | React.MouseEvent<HTMLButtonElement>) => { |
There was a problem hiding this comment.
form태그 안에 버튼은 자동적으로 type='submit'이 되며 onClick이벤트를 달지 않아도 form의 onSubmit 동작이 작동합니다. 따라서
| React.MouseEvent<HTMLButtonElement> 타입지정은 불필요하며 버튼에 달린 onClick과 type='submit'도 불필요합니다.
| } | ||
| } | ||
| } | ||
| useEffect(() => {}, [loginButtonDisabled]) |
There was a problem hiding this comment.
useEffect(() => {}, [loginButtonDisabled])삭제하지 않은 코드같습니다.
| /> | ||
| <button | ||
| className={`${style.signupButton} ${ | ||
| loginButtonDisabled.every((check) => check === true) ? '' : style.disabled |
There was a problem hiding this comment.
:disabled로 스타일링이 되어있기 떄문에 따로 삼항연산자 필요없습니다.
| const token = getCookie('token') || '' | ||
| const user = JSON.parse(localStorage.getItem('user') || '{}') | ||
| useEffect(() => { | ||
| if (token && user.role) { | ||
| alert('이미 로그인 되어있습니다') | ||
| navigate(-1) | ||
| } | ||
| }, []) |
There was a problem hiding this comment.
로그인이 된 상태에서 접근을 제한하는 로직인것 같네요.
만약 로그아웃 상태여야만 접근이 가능한 라우트가 많아질 경우 해당 로직을 복사 붙여넣기를 해야합니다.
protected route를 만드는것과 유사하게 다음과 같이 만들 수 있습니다.
// (LogoutRequireRoute.tsx)
import { Outlet, Navigate } from 'react-router-dom'
import { getCookie } from '@/utils/cookie'
export const LogoutRequireRoute = () => {
const token = getCookie('token')
if (token) {
return <Navigate to="/" />
}
return <Outlet />
}
// (App.tsx)
...
<Route element={<LogoutRequireRoute />}>
<Route path="/login" element={<LogIn />} />
<Route path="/signup" element={<SignUp />} />
</Route>
...There was a problem hiding this comment.
현재 로그아웃 상태여야만 접근 가능한 페이지는 두 개 밖에 없어서 따로 privateRoute처리를 하지는 않았습니다. 사실 privateRoute처리를 모르는 상태였어서 해당 로직을 먼저 구현하고 나머지 로그인이 필요한 라우트들에 privateRoute처리를 한 것이라서 굳이..? 싶은 마음에 귀찮음이 더 컸었던 기억이 나네요..😂 확장성과 가독성을 고려하면 따로 처리하는게 확실히 깔끔할 것 같아요! 리팩토링 하면서 반영하도록 하겠습니다!
There was a problem hiding this comment.
관리자 사이드 바이므로 파일명을 AdminSidebar.tsx 가 적절해 보입니다.
일반 사원의 사이드바의 경우 MainHeader.tsx 컴포넌트 던데 이름 바꾸는게 좋아보입니다.,.
| <ul className={styles.menu__tab}> | ||
| <li> | ||
| <NavLink | ||
| to="/mypage/annual" |
There was a problem hiding this comment.
NavLink 의 to속성 없어도 컴포넌트의 전환이 이뤄집니다.
routing 사용하실거면 App.tsx에서 세팅하면 됩니다.
There was a problem hiding this comment.
handleTabClick()으로 컴포넌트 전환이 이루어지니 to 속성은 제거하겠습니다! routing의 경우 route 기반의 컴포넌트 전환은 구현해봤었는데, props 기반의 컴포넌트 전환도 가능하다고 하여 props 기반으로 도입해보게 되었습니다.
| const handleLink = (e: React.MouseEvent<HTMLDivElement, MouseEvent>) => { | ||
| const target = e.currentTarget as HTMLElement | ||
| if (target.innerText === '전체 일정 보기') { | ||
| navigate('/') | ||
| } else if (target.innerText === '마이페이지') { | ||
| navigate('/mypage') | ||
| } else { | ||
| navigate('/') | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
해당 로직은 불필요합니다.
<div
className={`${style.navItem} ${location.pathname === '/' ? style.active : ''}`}
onClick={() => navigate('/')}
>
<BsFillCalendarWeekFill size="20" />
<span style={{ marginLeft: '10px' }}>전체 일정 보기</span>
</div>
<div
className={`${style.navItem} ${location.pathname.includes('/mypage') ? style.active : ''}`}
onClick={() => navigate('/mypage')}
>
<BsPersonBadgeFill size="20" />
<span style={{ marginLeft: '10px' }}>마이페이지 </span>
</div>메뉴가 두개 밖에 안되기 때문에 handleLink로 e.target.innerText와 같은 작업은 과해보입니다.
만약 확장성 있는 코드를 만들고자 한다면
// 아이콘 importing 됩니다.
export const SIDE_MENU = [
{ label: '전체일정보기', icon: BsFillCalendarWeekFill, to: '/' },
{ label: '마이페이지', icon: BsPersonBadgeFill, to: '/mypage' },
{ label: '추가할페이지1', icon: BsPersonBadgeFill, to: '/addroute1' },
{ label: '추가할페이지2', icon: BsPersonBadgeFill, to: '/addroute2' }
]
{SIDE_MENU.map((menu) => (
<div
key={menu.label}
className={`${style.navItem} ${location.pathname === menu.to ? style.active : ''}`}
onClick={() => navigate(`${menu.to}`)}
>
<menu.icon size="20" />
<span style={{ marginLeft: '10px' }}>{menu.label}</span>
</div>
))}다음과 같이 사용하면 됩니다.
There was a problem hiding this comment.
이렇게도 사용할 수 있군요..! 기존 하드 코딩된 방식보다 훨씬 깔끔하고 수정이 용이하겠네요. 보면 바로 이해가 가는데 왜 제가 코드 짤 때는 생각이 안나는지.. 🥲
| //랜딩 시 유저 정보를 가져옵니다. | ||
| useEffect(() => { | ||
| getUserInfo() | ||
| }, []) | ||
| // 유저정보 가져오기 | ||
| const getUserInfo = () => { | ||
| setUser(JSON.parse(localStorage.getItem('user') || '{}')) | ||
| } |
There was a problem hiding this comment.
각자 담당 파트를 작업하면서 '상태 관리 툴이 필요하다고 느끼는 조원이 있다면 그 조원이 사용해본 경험이 있고 익숙한 툴을 도입하자'고 대화를 나누었지만 도입 없이 마무리를 하게 되었습니다. (저는 이번 프로젝트에 '상태 관리 tool 없이 props로 해보자'는 개인적인 목표로 도입하지 않았습니다!) 리팩토링 시 다른 조원분이 의견이 있으시다면 추가해보겠습니다:)
| import { SortedData } from "@/utils/SortedData"; | ||
|
|
||
| const AdminEmployee = () => { | ||
| const [data, setData] = useState<userInfo[]>([]); |
There was a problem hiding this comment.
admin/index.ts 파일에서 return [] 제거 하시면 이슈가 해결 될 거 같습니다.
return [] 때문에 에러가 일어나면 getUserList 함수의 반환 값은 빈 배열이 됩니다.
여기서 setData(response.data)를 호출하는데 response에는 data 속성이 없어서 undefined가 되어 오류가 발생하는거 같습니다.
다만 api는 여전히 400에러를 나타내고 있습니다.
| return res.data | ||
| } catch (error) { | ||
| console.error('사용자 목록 조회 api 오류', error) | ||
| return [] |
There was a problem hiding this comment.
return [] 삭제해도 무방할거 같습니다.
admin/employee 페이지가 렌더링 되지 않는 이유인거 같습니다.
| useEffect(() => { | ||
| async function fetchData() { | ||
| const token = getCookie('token') | ||
| const userList = await getUserListApi(token) |
There was a problem hiding this comment.
마찬가지로 getUserListApi가 400에러가 일어납니다.
postman으로 찍어봐도 똑같은 {"errorMessage":["begin 4, end 8, length 6"]} 에러 메세지를 반환합니다.
| onWorkAssigned: () => void | ||
| } | ||
|
|
||
| const AdminWork = ({ dateInfo, employees, setShowAdminWork, onWorkAssigned }: Props) => { |
There was a problem hiding this comment.
employees의 상태값이 getUserList 데이터를 받아 오지 못 해서 select option을 추가하지 못 하고 있습니다.
| <div | ||
| className={`${style.navItem} ${location.pathname === '/logout' ? style.active : ''}`} | ||
| onClick={() => { | ||
| handleLogout('/login', navigate) |
There was a problem hiding this comment.
위에 정우님이 말한 거 처럼 파라미터로 주지 않고, useNavigate를 사용하려면 handleLogout을 커스텀 훅으로 만들어서 쓰는 방법도 있는거 같습니다.
// handleLogout.ts
import { useNavigate } from 'react-router-dom'
import { logoutApi } from '@/api/user'
import { removeCookie, getCookie } from '@/utils/cookie'
export default const useLogout = () => {
const navigate = useNavigate()
const handleLogout = async () => {
try {
const token = getCookie('token')
const res = await logoutApi(token)
if (!res || res.status === 401) {
// 응답이 없거나 401 상태 코드를 받으면 토큰을 강제로 제거하고 사용자를 로그인 페이지로 이동
removeCookie('token')
localStorage.removeItem('user')
navigate('/login')
return
}
if (Array.isArray(res)) {
console.error(res[0])
} else {
removeCookie('token')
localStorage.removeItem('user')
alert(res.message)
navigate('/login')
}
} catch (error) {
console.error('로그아웃 실패', error)
removeCookie('token')
localStorage.removeItem('user')
navigate('/login')
}
}
return handleLogout
}
// MainHeader.tsx
import useLogout from '@/utils/handleLogout'
<div className={`${style.navItem} ${location.pathname === '/logout' ? style.active : ''}`}
onClick={useLogout()}>
<RiLogoutBoxFill size="20" />
<span style={{ marginLeft: '10px' }}>로그아웃</span>
</div>
| useEffect(() => { | ||
| if (token && user.role) { | ||
| alert('이미 로그인 되어있습니다') | ||
| navigate(-1) |
There was a problem hiding this comment.
navigate(-1)을 사용하시면, 유저가 이미 로그인 상태에서 다른 웹페이지로 이동했다가 /login에 접근할 경우 navigate(-1) 때문에 단순히 뒤로가기가 되어버리는거 같습니다. 좀 더 명확하게 경로를 지정해주시면 유저 이탈을 방지할 수 있을거 같습니다.
| useEffect(() => { | ||
| if (token && user.role) { | ||
| alert('이미 로그인 되어있습니다') | ||
| navigate(-1) |
| import styles from './MyDuty.module.scss' | ||
|
|
||
| // 당직 조회 탭을 출력하는 MyDuty component | ||
| const MyDuty: React.FC = () => { |
There was a problem hiding this comment.
: React.Fc를 사용하지 않는 추세라고 들었습니다.
https://react-typescript-cheatsheet.netlify.app/docs/basic/getting-started/function_components/
영문 커뮤니티 글이고,
https://story.pxd.co.kr/1650
그 글을 정리한 국내 블로그 입니다.
There was a problem hiding this comment.
헉 그렇군요! React 17 ver. 부터 타입 에러를 skip하는 군요...! 몰랐던 사실인데 참고하겠습니다! 감사합니다:D
| const MyDuty: React.FC = () => { | ||
| // 오늘, 내일 날짜 조회 | ||
| const currentDate = new Date() | ||
| const tomorrowDate = new Date(currentDate) |
There was a problem hiding this comment.
const currentDate = new Date()
const tomorrowDate = new Date(currentDate)
콘솔에 찍어보면 같은 날짜를 나타내고 있는데 이렇게 따로 분리해서 사용하는 이유가 있을까요?
There was a problem hiding this comment.
당직 조회 api의 경우 Response Data에 시간과 관련된 속성이 없게 백엔드측에서 설계 해주셨습니다. 프로젝트 초반 API 명세서를 함께 작성할 때에 계획했던 당직 조회 컴포넌트의 내용이 다소 부족하다는 생각이 들어 후반부에 페이지 레이아웃을 개인적으로 수정하게 되었습니다. 그래서 예정된 당직과 완료한 당직 일정을 구분하기 위해 변수에 저장하게 되었는데 현재 시간을 기준으로 계산하는 변수들이기 때문에 시간 설정에 의해 계속 변화하게 됩니다. tomorrowDate는 currentDate의 날짜와 시간 정보를 복사한 새로운 Date객체로, console에서는 같은 날짜로 출력되지만 서로 다른 Date 객체입니다:) 같은 객체를 참고하게 되면 데이터가 변경될 경우 current와 tomorrow가 모두 변경될 수 있기 때문에 각자 다른 Date 객체로 독립적으로 관리하게 하였습니다!
There was a problem hiding this comment.
MyDuty 컴포넌트에서 tomorrow는 선언되고 tomorrowDate.setDate(tomorrowDate.getDate() + 1) 초기화 된 이후로 사용되지 않은거 같은데 확인 가능하실까요?
그리고 개인적으로 useEffect 종속성 배열에서 selectedYear가 없어도 정상적으로 동작하지 싶습니다.
년도 자체를 이동하는거면 모르겠으나, 현재는 selectedMonth의 상태값이 변하는 것 만으로도 충분할거 같습니다.
| const token = getCookie('token') | ||
| await registWorkApi(token, data) |
There was a problem hiding this comment.
해당 부분에서 토큰을 가져와서 api 쏘는 코드로 보입니다
즉 토큰이 있는 api에서는 계속 토큰을 가져오는 로직이 필요해보이는데
토큰을 axios 인스턴스에 넣거나, axios 인터럽트를 사용하시면 코드가 더 간결해질꺼같습니다~
| const assignHandler = async () => { | ||
| const annualData = await getAnnualApi( | ||
| parseInt(dateInfo.dateStr.slice(0, 4)), | ||
| parseInt(dateInfo.dateStr.slice(5, 7)) | ||
| ) | ||
|
|
||
| for (const employee of selectedEmployees) { | ||
| if (!employee) continue | ||
|
|
||
| const foundEmployee = employees.find((e) => `${e.name}${e.employeeNumber.slice(0, 5)}` === employee) |
There was a problem hiding this comment.
assignHandler 에서 수행하는 로직이 많이 복잡하고 긴거같습니다
우선 추후 유지보수를 위해 함수는 각각 독립적인 역할을 하는 함수로 분리해주시는걸 추천드리고
이런 복잡한 함수는 useMemo 혹은 useCallback으로 묶어주시면 좋습니다~
| const applyHandler = async () => { | ||
| //연차 신청 전에, 전체 연차 개수가 15개 이상인지 확인 | ||
| const annualCount = myAnnual.length | ||
| if ((annualCount as number) >= 15) { |
There was a problem hiding this comment.
여기서 number 로 형변환하는것을 불필요해보입니다 length 로 반환된 데이터는 무조건 number이기 때문에 형변환은 제거해도 될꺼같습니다~
| alert('오늘 이전 날짜는 선택할 수 없습니다.') | ||
| return | ||
| } | ||
| const isAnnualExist = myAnnual.find((item) => item.date === e.target.value) |
There was a problem hiding this comment.
myAnnual.find 가 dateValueHandler 에도 있고 applyHandler에도 있는거 같습니다 이 부분을 별도의 함수로 만들면 코드 관리가 더 용이해질꺼같습니다~
const hasAppliedForAnnualLeave = (date: string) => myAnnual.some(item => item.date === date);
| if (annualData) { | ||
| setMyAnnual(annualData) | ||
| annualData | ||
| .filter((item) => item.status === 'APPROVED') |
There was a problem hiding this comment.
APPROVED,CANCELED,UNAPPROVED 와 같은 데이터가 다른 컴포넌트에서도 사용되고있는거같습니다
이러한 데이터는 별도의 상수 데이터로 관리해주시는게 좋습니다~
| const currentDate = new Date() | ||
|
|
||
| // 메인 캘린더에서 신청한 연차 중 아직 승인되지 않은 연차 목록 Filtering | ||
| const requestedAnnualData = filteredAnnualData.filter((annual) => annual.status === 'UNAPPROVED') |
There was a problem hiding this comment.
마찬가지로 이러한 filter 로직도 src/components/mypage/RemainingAnnual.tsx 컴포넌트에 있는 코드인거같습니다
이부분도 커스텀 훅으로 빼주시는걸 추천드립니다~
| useEffect(() => { | ||
| const fetchData = async () => { | ||
| const token = getCookie("token"); | ||
| const response = await getAnnualAdminApi(token); | ||
|
|
||
| if (response) { | ||
| setData({ data: response.data }); | ||
| } | ||
| }; | ||
|
|
||
| fetchData(); | ||
| }, [approvedId, rejectedId]); |
There was a problem hiding this comment.
다음 프로젝트에서는 이러한 fetching 하는 로직을 react-query로 구성해보시는것을 추천드립니다
| // 연차 신청 목록에 있는 사원 검색 | ||
| const filteredData1 = data.data | ||
| .filter((item) => item.status === "UNAPPROVED") | ||
| .filter((employee) => employee.name.toLowerCase().includes(delayedSearch1.toLowerCase())); | ||
| // 취소 신청 목록에 있는 사원 검색 | ||
| const filteredData2 = data.data | ||
| .filter((item) => item.status === "CANCELED") | ||
| .filter((employee) => employee.name.toLowerCase().includes(delayedSearch2.toLowerCase())); |
There was a problem hiding this comment.
로직은 동일하고 로직 내에 데이터만 달라보입니다 이 로직을 아래처럼 함수로 관리가능할꺼같습니다~
이렇게 함수로 뺴주시면 useMemo 혹은 useCallback을 사용하기에 더 유리합니다~
const filterEmployees = (data, status, searchTerm) => {
return data
.filter(item => item.status === status)
.filter(employee => employee.name.toLowerCase().includes(searchTerm.toLowerCase()));
};
| currentDate.setHours(0, 0, 0, 0) // 오늘 | ||
| tomorrowDate.setDate(tomorrowDate.getDate() + 1) // 내일 |
There was a problem hiding this comment.
이러한 초기값 설정은 useEffect(()=>{},[]) 에 넣어주시는걸 추천드립니다
작성해주신 코드대로 보면 사이드 이펙트가 발생할 여지가 있을꺼같습니다~
There was a problem hiding this comment.
이제보니 그럴 것 같습니다😭 컴포넌트 내에서 선언된 변수들이 컴포넌트 재렌더링 시 다시 재계산 된다는 점을 자주 놓치는 것 같습니다.. 리팩토링 시 적용하고 다음엔 꼭 놓치지 않도록 신경 써보겠습니다!!
| const [activeTab, setActiveTab] = useState<'annual' | 'duty' | 'info'>('annual') | ||
|
|
||
| // 연차 관련 데이터 저장 | ||
| const [annualData] = useState<annuals[]>([]) | ||
|
|
||
| // 당직 관련 데이터 저장 | ||
| const [dutyData] = useState<works[]>([]) | ||
|
|
||
| // MenuTab 클릭 시 Tab 전환 | ||
| const handleTabClick = (tab: 'annual' | 'duty' | 'info') => { |
There was a problem hiding this comment.
'annual' | 'duty' | 'info' 이 중복되고있습니다 이러한 부분은 별도의 type 으로 빼주시면 좋습니다~
There was a problem hiding this comment.
반복적으로 사용된 hook과 type들을 분리하여 사용해보겠습니다! 아직 이런 디테일한 부분이 많이 부족한 것 같습니다😭 감사합니다!
📑 당연해 (DangYeonHae)
React, TypeScript, Rest API를 활용한 연차 / 당직 관리 웹사이트 입니다.
📌 프로젝트 소개
> 관리자 계정
📌 개발 팀원 및 역할
초기 개발 세팅
메인 페이지
로그인, 회원가입, 로그아웃
Header
(사원 관리, 연차 관리, 당직 관리)
(연차 조회, 당직 조회, 비밀번호 수정)
📌 사용 기술 및 개발 환경
Development
Config
Deployment
Environment
Cowork Tools
📌 프로젝트 테스트
clone project
install npm
start project
📌 프로젝트 상세 기능
1️⃣ 메인 페이지
HEADER
Calendar
Login / SignUp
2️⃣ 마이 페이지
연차 조회
당직 조회
비밀번호 수정
3️⃣ 관리자 페이지
사원 관리
당직 관리
연차 관리
사원명, 연차 날짜 출력, 관리자가 취소신청의 승인/거부 가능
기타 기능
📌 프로젝트 구조
보기
📌 구현 화면