5조 과제 제출 (안중후, 이정환, 김하은) #2
Conversation
Choi yong jun/mypage list
There was a problem hiding this comment.
안녕하세요 5조 여러분! 미니 프로젝트 진행하시느라 너무나 고생 많으셨습니다.
객관적인 시선으로도 ㅎㅎ 저보다 훌륭하신 분들이라 특별히 기능 오류를 찾기 보다는
제 선에서 도움 드릴 수 있는 디테일한 측면으로 리뷰해보았는데 도움이 되실지 모르겠네요.
코드를 보면서 느낀 점은 파일을 너무 잘 분리하셔서 컴포넌트 내 코드들이 깔끔하다는 점이 인상 깊었습니다.
저의 의견은 작은 부분이므로 모쪼록 리팩토링 잘 진행하셔서 더 나은 프로젝트로 거듭났으면 하는 바람입니다.
고생 많으셨고 남은 파이널도 힘내시기 바랍니다! 감사합니다 😉💖
| const authInterceptors = (instance: AxiosInstance): AxiosInstance => { | ||
| instance.interceptors.request.use( | ||
| config => { | ||
| // 로컬스토리지에 저장되어 있는 AccessToken을 호출. | ||
| const accessToken = localStorage.getItem('token') | ||
| if (config.headers && accessToken) { | ||
| // accessToken이 정상적으로 저장 => headers에 authorization 값을 추가. | ||
| config.headers.Authorization = `${accessToken}` | ||
| } | ||
| // authorization을 추가한 config 반환 | ||
| return config | ||
| }, | ||
| (error: AxiosError): Promise<AxiosError> => { | ||
| return Promise.reject(error) | ||
| } | ||
| ) | ||
|
|
||
| return instance | ||
| } |
There was a problem hiding this comment.
Axios Interceptors 사용하셨군요!
저는 생각만해보고 적용하지는 못했는데 직접 사용하신 코드 보니 다음 프로젝트 때 써봐야겠다는 생각을 하게 됩니다 ㅎㅎ
There was a problem hiding this comment.
시간 안에 NotFound 페이지까지 구현하신 디테일에 놀라고 갑니다!
다른 컴포넌트들과 달리 스타일 코드가 상단에 적혀있는데 해당 파일만 스타일 코드의 위치가 다른 이유가 있나요?
There was a problem hiding this comment.
저번에 써놓은 코드 복붙했습니다.... 담엔 복붙도 센스있게 해보겠습니다 ㅎ...!
| } catch (err) { | ||
| // 예외처리 서버에서 완료, alert로만 예외처리 | ||
| if (err instanceof AxiosError) { | ||
| alert(`${signupTexts.requiredData}`) | ||
| } |
There was a problem hiding this comment.
이메일 중복 확인을 제외하고는 원하는 Validation을 충족하지 못했을 때 사용자에게 알려줄 수 있는 처리들이 부족하지 않나 생각이 듭니다.
"회원가입에 실패했습니다. 입력항목을 다시 확인해주세요." 문구로는 사용자가 어떤 항목에서 조건이 미달됐는지 체크하기 어렵겠다는 생각이 들었습니다 ㅎㅎ
하나의 input마다 프론트 측에서도 서버와 함께 예외 처리를 해주는 것이 어떨까요?
There was a problem hiding this comment.
저도 그 부분에 대해 고민했는데 보안상 구체적인 오류를 알려주는 게 좋지않다는 포스트를 본 것 같아 그렇게 처리해둔 것 같습니다! (e.g. 비밀번호가 틀렸다는 알림시 보안공격에 구체적인 단서를 제공)
| <DatePickerForm | ||
| date={startDate} | ||
| setDate={setStartDate} | ||
| /> |
There was a problem hiding this comment.
데이트 픽커가 input창 기준 상단에 위치하고 있어서 5주차가 넘어가는 특정 달에서는 사용자가 누를 수 있는 픽커 버튼의 위치가 이동되기 때문에 다량의 클릭이 필요한 부분에서 불편함을 느꼈습니다. 해당 부분을 테스트 해보시고 더 나은 사용자 경험을 유도할 수 있는 방향으로 리팩토링 해보시면 좋을 것 같습니다 :)
There was a problem hiding this comment.
넵 좋은 의견 감사합니다!! 수정해서 반영해보도록 하겠습니다 :)!!
| const CalendarContainer = styled.div` | ||
| width: 100%; | ||
| padding-bottom: 5%; | ||
| background-color: #ffffff; | ||
| position: absolute; | ||
| border: 2px solid #696ea6; | ||
| box-shadow: #50515985 1px 2px 7px 1px; | ||
| border-radius: 10px; | ||
| ` | ||
|
|
||
| const CalendarBox = styled.div` | ||
| width: 95%; | ||
| position: relative; | ||
| margin: 0 auto; | ||
| top: 20px; | ||
| border-radius: 10px; | ||
| font-family: 'LINESeedKR-Bd'; |
There was a problem hiding this comment.
Styled-Components의ThemeProvider를 사용하신만큼 공통 스타일 속성을 더 디테일하게 관리해보시면 좋을 것 같습니다. 만들어두신 테마가 있음에도 불구하고 스타일 값들이 하드코딩으로 들어있는 부분은 수정해주시면 좋을 것 같습니다!
| import { styled } from 'styled-components' | ||
|
|
||
| export const Footer = () => { | ||
| return <FooterContainer></FooterContainer> | ||
| } | ||
|
|
||
| const FooterContainer = styled.div` | ||
| width: 100%; | ||
| /* background-color: #444141; | ||
| padding-bottom: 70px; */ | ||
| ` |
There was a problem hiding this comment.
프로젝트 데모에서는 Footer의 특별한 기능이나 필요성을 발견하지는 못했는데 어디에 사용되었는지 알 수 있을까요?
There was a problem hiding this comment.
푸터를 사용할 것이라고 생각해두고 임시작성해둔 파일인데 지워야할 것 같네요...!
| export const SignUpForm = () => { | ||
| const navigate = useNavigate() | ||
| // 유저 입력 데이터들 | ||
| const [name, setName] = useState<string>('') | ||
| const [email, setEmail] = useState<string>('') | ||
| const [password, setPassword] = useState<string>('') | ||
| const [verification, setVerification] = useState<string>('') | ||
| const [startDate, setStartDate] = useState(new Date()) | ||
| // 이메일 중복확인 데이터 | ||
| const [isEmailInUse, setIsEmailInUse] = useState(true) |
There was a problem hiding this comment.
여러 state를 활용해야 할 때 주석으로 데이터를 분리하여 체크할 수 있도록 하신 것이 함께 하는 팀원이나 나중을 위해서도 너무 좋은 것 같습니다! 저도 꼼꼼히 주석을 달아야겠단 생각을 하게 되네용!
| export const signinTexts = { | ||
| title: '당연하지', | ||
| email: '이메일', | ||
| pwd: '비밀번호', | ||
| btn: '로그인', | ||
| forgotPwd: '비밀번호 찾기', | ||
| signup: '회원가입 하기', | ||
| emailPh: '이메일을 입력해주세요.', | ||
| pwdPh: '비밀번호를 입력해주세요', | ||
| alertText: '비밀번호를 다시 확인해주세요.' | ||
| } |
There was a problem hiding this comment.
특정 기능 구현 시 필요한 text들을 모아서 한 번에 관리하는 아이디어가 너무 새롭네요!!
저는 한 번도 생각하지 못한 방법인데 배우고 갑니다. 컴포넌트 내의 코드는 간결해지고 유지보수 시에도 좋을 것 같습니다. 🤪👍
| export const router = createBrowserRouter([ | ||
| { | ||
| path: '/', | ||
| element: <Layout />, | ||
| errorElement: <ErrorComponent />, | ||
| children: [ | ||
| { | ||
| path: '/', | ||
| element: <SignIn />, | ||
| errorElement: <ErrorComponent /> | ||
| }, | ||
| { | ||
| path: '/signup', | ||
| element: <SignUp />, | ||
| errorElement: <ErrorComponent /> | ||
| }, | ||
| { | ||
| path: '/reset', | ||
| element: <ResetPassword />, | ||
| errorElement: <ErrorComponent /> | ||
| } | ||
| ] | ||
| }, | ||
| { | ||
| path: '/', | ||
| element: <HeaderLayout />, | ||
| errorElement: <ErrorComponent />, | ||
| children: [ | ||
| { | ||
| path: '/home', | ||
| element: <MainHome />, | ||
| errorElement: <ErrorComponent /> | ||
| }, | ||
| { | ||
| path: '/profile', | ||
| element: <UpdatePage />, | ||
| errorElement: <ErrorComponent /> | ||
| }, | ||
| { | ||
| path: '/schedule', | ||
| element: <SchedulePage />, | ||
| errorElement: <ErrorComponent /> | ||
| }, | ||
| { | ||
| path: '/application', | ||
| element: <ApplyPage />, | ||
| errorElement: <ErrorComponent /> | ||
| }, | ||
| { | ||
| path: '/logout', | ||
| element: <SchedulePage />, | ||
| errorElement: <ErrorComponent /> | ||
| } |
There was a problem hiding this comment.
ErrorElement를 따로 관리해주신 디테일 배우고 갑니다 !! 👍
한 가지 드는 궁금증은 각 페이지마다 모두 ErrorElement가 필요한데 한 번에 핸들링하여 코드의 반복을 줄일 수 있는 방법은 없을까요?
There was a problem hiding this comment.
그건 저도 생각지못한 부분이네요! 중첩라우팅시 최상단에만 errorElement를 작성시 children페이지들에서는 자동으로 적용이 될 것 같다는 생각도 드는데 만약 그렇다면 다음번부터는 굳이 작성할 필요가 없을 것 같습니다!! 덕분에 중복을 줄일 수 있는 방법에 대해 생각해볼 수 있는 계기가 된 것 같습니다:)!
| import { Schedule } from 'components/schedule/index' | ||
|
|
||
| export const SchedulePage = () => { | ||
| return ( | ||
| <> | ||
| <Schedule /> | ||
| </> | ||
| ) | ||
| } |
There was a problem hiding this comment.
Schedule 페이지에는 같은 이름의 컴포넌트 한 개의 요소만 들어있고 해당 요소를 감싸는 별도의 레이아웃 스타일 처리도 없는 것 같은데, 혹시 한번 더 감싼 이유(?) 혹은 컴포넌트로 특별히 분리하신 이유가 궁금합니닷!
| export const applyAnnual = async (updatedData: any) => { | ||
| const APPLYANNUALURL = '/vacation/add' | ||
| const res = await authInstance.post(APPLYANNUALURL, updatedData) | ||
| return res | ||
| } |
There was a problem hiding this comment.
api 호출 안에서 url을 따로 선언해서 사용하는건 처음 봅니다!
특별한 이유가 있어서 이렇게 구성하신건지 궁금합니다!
호출하는 줄이 길어지지 않아 깔끔하게 정리된 것 같아서 너무 보기 좋은것 같아요!! 배워갑니다 👍👍
| const handleChange = (e) => { | ||
| console.log(e.target.value) | ||
| setReason(e.target.value); | ||
| } | ||
|
|
||
| const submitButton = () => { | ||
| selectedDate.setHours(9, 0, 0, 0); | ||
| const isDuplicateDate = data.filter((item) => { | ||
| console.log(item) | ||
| const startDay = item.start; | ||
| const endDay = item.end; | ||
| startDay.setHours(9,0,0,0) | ||
| endDay.setHours(9,0,0,0) | ||
| endDate.setHours(9,0,0,0) |
There was a problem hiding this comment.
코드가 깔끔해서 중간중간 console.log들이 잘보이네요.!!! 👍
리팩토링하실때 충분히 신경 쓰실 것 같지만
리마인드 차원에서 한 번 말씀 드려봅니당..!
| //가장 바깥 흰 배경 바탕 레이아웃 | ||
| const Outermost = styled.div` | ||
| background-color: #fff; | ||
| display: flex; | ||
| justify-content: center; | ||
| align-items: center; | ||
| width: 100%; | ||
| height: 100px; | ||
| position: fixed; | ||
| z-index: 200; | ||
| box-shadow: 0px 4px 6px rgba(0, 0, 0, 0.04); | ||
| ` |
There was a problem hiding this comment.
스타일드 컴포넌트 사용할 때 주석 달아볼 생각을 못했는데
굳이 위로 올라가서 확인하지 않아도 돼서 좋은것 같아요!
배우고 갑니다...! 👍👍
| export const AllDataList = ({CalendarDate, annualData, dutyData}) => { | ||
|
|
||
| const calendarRef = useRef<FullCalendar | null>(null); | ||
| const [CalDate, setCalDate] = useState<number>(2023); |
There was a problem hiding this comment.
달력의 년도와 관련이 있는것 같은데 동적으로 해당 년도를 받아오는게 아닌
2023이라고 초기에 넣어 놓으신 특별한 이유가 있는지 궁금합니다!
| text-decoration: none; | ||
| } | ||
|
|
||
| @font-face { |
There was a problem hiding this comment.
폰트를 html에서 불러오면 렌더링 성능에 더 유리하다고 들었어요. css내에서 Import하는 데에 큰 이유가 있지 않으시다면 렌더링 최적화를 위해 html head 내부에서 폰트 import 하는 것을 추천드려봅니다!👀
There was a problem hiding this comment.
꿀팁 감사합니다 승이님! 이런 디테일한 부분에서 다듬어지는 게 역시 코드리뷰의 장점같네요👍👍👍🙌🏻🙌🏻
| </> | ||
| ) | ||
| } | ||
| const ServiceImage = styled.img`` |
There was a problem hiding this comment.
스타일링이 불필요한 타이틀 이미지 같은 경우, styled 선언은 불필요해보이는 것 같아요. 다만 return문 내의 가독성 부분에는 좋아 보이는데, 이 부분은 한 번 생각해볼만한 것 같네요! 💭
There was a problem hiding this comment.
앗 이 부분은 정의만 위에 해두고 아래에서 스타일을 상위 스타일 컴포넌트 SigiInWrapper하에 중첩 적용했습니다!!
| ,type : "" | ||
| }]); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
useEffect의 두 .then 문을 async/await를 통해 작성하면 조금 더 가독성 높은 코드를 사용하실 수 있을 것 같다고 생각이 들어요! 그리고 긴 함수의 경우 개별 함수로 분리하는 것도 추천드립니다.👍
| } | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
단순 else if 문을 사용하는 것이 아닌 switch-case 문을 사용하신게 좋았습니다! 배웠는데도 항상 사용하는데엔 쓰던 것만 쓰게 되는데, 이렇게 적절한 용도에 switch를 사용하신 것을 보니 저도 나중에 사용해보면 좋을 것 같다는 생각 드네요! 👍
There was a problem hiding this comment.
#총평
디자인이 아주 좋습니다.
신기할 정도로...
제안사항
- 라인의 끝에 세미콜론을 붙이는 것은 선택이지만, 팀의 코드라면 통일하여 붙일거면 다 붙이고 붙이지 않을 거라면 아예 안 붙이는 것이 좋습니다.
- 컴포넌트가 너무 길어지는 경우가 많습니다. 분리할 수 있는 부분은 최대한 분리해주세요. 컴포넌트도 하나의 함수일 뿐입니다.
- 스타일도 컴포넌트 외 다른 파일로 빼내면 컴포넌트의 길이를 단축시킬 수 있습니다.(취향차이)
마무리...
이번 과정에서 제가 여러분께 드리는 마지막 코드리뷰고, 여러분이 받는 마지막 코드리뷰겠네요.
제가 한 모든 코드리뷰는 한 사람의 의견으로 듣되, 왜 이런 의견이 나왔는지 고심해 보시길 바랍니다.
짧은 시간 동안 제가 여러분들의 사수 역할을 조금이나마 해냈다면 좋겠네요.
마지막 프로젝트는 아니지만 그 동안 고생하셨고, 여러분이 노력하신 만큼 결과가 뒤따라오길 기원하겠습니다.
다음에 뵐때는 멘토와 수강생이 아니라 개발자와 개발자로 뵙겠습니다.
수고하셨습니다.
| export const checkEmailAvailable = async (email: string) => { | ||
| const res = await baseInstance.get(`/emailCheck?email=${email}`) | ||
| return res.data | ||
| } |
| import { authInstance } from './axios' | ||
|
|
||
| export const applyAnnual = async (updatedData: any) => { | ||
| const APPLYANNUALURL = '/vacation/add' |
There was a problem hiding this comment.
단어 사이를 로대쉬(_)로 구분해 주는 것이 좋아보입니다.
| setReason(e.target.value); | ||
| } | ||
|
|
||
| const submitButton = () => { |
There was a problem hiding this comment.
submitButton이라는 네이밍은 실제 버튼을 나타내는 것 같습니다.
submit 액션이라는 것을 더 명시적으로 나타내는 네이밍으로의 변경이 필요해보입니다.
| const [startDate] = useState(selectedDate || new Date()); | ||
| const [endDate, setEndDate] = useState(selectedDate || new Date()); | ||
| const [getReason, setReason] = useState("연차") | ||
| const [ViewData] = useState({ |
There was a problem hiding this comment.
setState를 안사용하는 변수는 굳이 state로 선언해줄 필요가 없습니다.
| const [selectedDate, setSelectedDate] = useState<Date | null>(null); | ||
| const [CalDate, setCalDate] = useState<number>(2023); | ||
| const [username, SetUserName] = useState(""); | ||
| const [data, setData] = useState<DataItem[]>([]); |
There was a problem hiding this comment.
단순한 data는 너무 애매한 네이밍이네요.
조금 더 명시적으로 정해주세요~
| } | ||
|
|
||
| try{ | ||
| if(type == "연차"){ |
| export const UpdateTexts = { | ||
| update: '사원 정보 수정', | ||
| profile: '프로필 사진', | ||
| account: '계정', | ||
| changePwd: '비밀번호 변경', | ||
| email: '이메일', | ||
| username: '이름', | ||
| newPwd: '새로운 비밀번호', | ||
| newPwdCheck: '비밀번호 확인', | ||
| cancel: '취소', | ||
| confirm: '등록', | ||
| delete: '삭제', | ||
| upload: '수정', | ||
| newPwdPh: '영어 대문자, 영어 소문자, 숫자, 특수문자를 모두 포함 (8글자 이상)', | ||
| newPwdCheckPh: '새로운 비밀번호를 한번 더 입력해주세요.', | ||
| verification: '새로운 비밀번호와 비밀번호 확인을 동일하게 입력해주세요.', | ||
| success: '회원정보가 수정되었습니다. 변경된 비밀번호로 로그인해주세요.', | ||
| failure: '변경할 비밀번호를 조건에 맞게 입력해주세요.' | ||
| } |
There was a problem hiding this comment.
진짜~~~너무 좋습니다.
프로젝트에서 하드코딩으로 텍스트를 넣은 행위는 지금처럼 최대한 지양해주세요.
다만, 상수의 경우 변수명을 UPDATE_TEXTS 등으로 대문자 스네이크 케이스로 지정해주세요.
| <> | ||
| <Apply /> | ||
| </> |
There was a problem hiding this comment.
어차피 리턴하는 최상위 DOM 이 하나이므로 굳이 Fragment tag를 사용할 필요가 없습니다.
| export const router = createBrowserRouter([ | ||
| { | ||
| path: '/', | ||
| element: <Layout />, | ||
| errorElement: <ErrorComponent />, | ||
| children: [ | ||
| { | ||
| path: '/', | ||
| element: <SignIn />, | ||
| errorElement: <ErrorComponent /> | ||
| }, | ||
| { | ||
| path: '/signup', | ||
| element: <SignUp />, | ||
| errorElement: <ErrorComponent /> | ||
| }, | ||
| { | ||
| path: '/reset', | ||
| element: <ResetPassword />, | ||
| errorElement: <ErrorComponent /> | ||
| } | ||
| ] | ||
| }, | ||
| { | ||
| path: '/', | ||
| element: <HeaderLayout />, | ||
| errorElement: <ErrorComponent />, | ||
| children: [ | ||
| { | ||
| path: '/home', | ||
| element: <MainHome />, | ||
| errorElement: <ErrorComponent /> | ||
| }, | ||
| { | ||
| path: '/profile', | ||
| element: <UpdatePage />, | ||
| errorElement: <ErrorComponent /> | ||
| }, | ||
| { | ||
| path: '/schedule', | ||
| element: <SchedulePage />, | ||
| errorElement: <ErrorComponent /> | ||
| }, | ||
| { | ||
| path: '/application', | ||
| element: <ApplyPage />, | ||
| errorElement: <ErrorComponent /> | ||
| }, | ||
| { | ||
| path: '/logout', | ||
| element: <SchedulePage />, | ||
| errorElement: <ErrorComponent /> | ||
| } | ||
| ] | ||
| } | ||
| ]) |
There was a problem hiding this comment.
규모가 큰 프로젝트라면 실제 라우팅이 이것보다 훨씬 복잡해질 수 있습니다.
배열 등을 이용해서 라우팅도 하드코딩이 아닌 코드 방식으로 변경할 수 있습니다.
| return ( | ||
| selected.getFullYear() + | ||
| '-' + | ||
| (selected.getMonth() + 1 < 9 | ||
| ? '0' + (selected.getMonth() + 1) | ||
| : selected.getMonth() + 1) + | ||
| '-' + | ||
| (selected.getDate() < 9 ? '0' + selected.getDate() : selected.getDate()) + | ||
| 'T' + | ||
| '09:00:00' + | ||
| 'Z' | ||
| ) |
There was a problem hiding this comment.
컴포넌트 뿐 아니라 이런 일반 함수에서도 리턴문이 길어지는 것은 좋지 않습니다.
const year = selected.getFullYear() 등으로 리턴문을 깔끔하게 만들어 줄 수 있습니다.
FE_Client
🐻❄ 프로젝트 소개
📚 STACKS
Config
Development
Deployment
💬 Communication
👨👩👧👦 팀원 역할
초기 개발 세팅
프로필 수정,
로그인, 회원가입,
Header, 비밀번호 재설정
(연차/당직 승인 페이지,
연차/당직 신청 페이지,
관리자 - 전체 유저 / 회원가입 요청 리스트 페이지 )
(홈페이지, 내 일정보기, 연차/당직 신청)
🖥 기능 소개
🧙🏻♂️ 로그인, 회원가입 페이지 구성
🧙🏻♀️ Side Bar - 네비게이션 페이지 구성
🧙🏻 관리자 권한 페이지 구성
1️⃣ 유저/관리자 로그인 페이지
2️⃣ 회원가입 페이지
3️⃣ 비밀번호 재설정 페이지
4️⃣ 전체 홈페이지
5️⃣ 프로필 수정 페이지
6️⃣ 내 일정보기 페이지
7️⃣ 연차/당직 신청 페이지
8️⃣ 관리자 - 전체 유저 / 회원가입 요청 리스트 페이지
어드민 계정을 입력하지않고 다른 페이지로 이동할 경우 토큰 값이 없으면 로그인페이지로 이동
사용자 측 회원가입 요청 리스트로 출력
사용자 측 회원가입시 관리자 승인 기능
전체 가입된 유저 리스트 출력
9️⃣ 관리자 - 연차/당직 승인 페이지
연차
당직
검색 기능
🔟 관리자 - 연차/당직 리스트 페이지
연차
당직
공통