Conversation
[feat] 정정페이지 완료
…eam1 into feat/salary-api-#153
[design] 전체적인 스타일 수정
[feat] 급여명세서 저장된 이미지에서 저장 버튼 제거
[feat] 급여명세서 api연동
[fix] 프로필 수정 시, 이미지 초기화 되는 버그 수정
[refactor] 최종 회의 중 발생한 문제 수정
[refactor] 최종테스트 중 발견한 사항 수정 누락부분
[fix] 하루 종일 일정이 오전12:00-오후11:59로 뜨는 오류 해결
[doc] 최종 README 업로드
[merge] 최종develop버전 main브랜치로 merge
iamidlek
left a comment
iamidlek
left a comment
There was a problem hiding this comment.
고생하셨습니다.
실제 배포된 사이트 혹은 로컬에서 확인 할 수 있게 환경 변수를 공유주시면
리뷰할 때 더 자세히 볼 수 있을 것 같습니다.
반복된 부분은 코멘트 생략하였습니다.
git commit 컨벤션과 코드 분리 및 스타일 작성, 컴포넌트화를 잘 해주신 것 같습니다.
다만 작업자마다 다른 코드 스타일이 보이는 부분이 일부 있고
로직에 대해 에러가 발생 가능한 부분이 있는 점
불필요한 state가 존재하는 등 개선되었으면 하는 부분도 있었습니다.
시간에 대한 서버와 클라이언트의 처리 방법이나
예외 사항에 대한 처리나 실시간 데이터 변경 반영이나
라이브러리 없이 캘린더 만들어 보기 등
개선 사항을 찾아 개선해 보아도 좋을 것 같습니다.
| import { firebaseConfig } from '@/constants/api'; | ||
|
|
||
| export const checkAuth = (): boolean => { | ||
| const sessionKey = `firebase:authUser:${firebaseConfig.apiKey}:${auth.app.name}`; |
There was a problem hiding this comment.
firebaseConfig.apiKey 를 조회하여 클라이언트에게 내려주는 것은
env로 서버쪽에 두어 감추어두는 의미가 없어질 것 같습니다.
| }; | ||
|
|
||
| export const getUID = async (): Promise<string> => | ||
| await auth.authStateReady().then(() => (auth.currentUser ? auth.currentUser.uid : '')); |
There was a problem hiding this comment.
auth.currentUser?.uid ?? ''
과 같이 작성하여도 좋을 것 같습니다.
| const PrivateRoute = () => { | ||
| const { pathname, search } = useLocation(); | ||
|
|
||
| return checkAuth() ? <Outlet /> : <Navigate to='/signin' replace state={pathname + search} />; |
There was a problem hiding this comment.
Navigate to='/signin'
이 부분만 PATH에서 값을 가져오지 않고 작성한 이유가 있을까요?
일관성 있게 관리되면 좋을 것 같습니다.
There was a problem hiding this comment.
작성하다가 놓친부분 같습니다! 유의하겠습니다!
|
|
||
| useEffect(() => { | ||
| if (checkAuth()) navigate(PATH.HOME); | ||
| }, [status]); |
There was a problem hiding this comment.
eslint react-hooks/exhaustive-deps 에러가 나오고 있는데
제 환경에서만 잡히는 에러일까요?
There was a problem hiding this comment.
저는 확인되지 않습니다.. 다른분들은 어떠신가요?
There was a problem hiding this comment.
status 부분에서 React Hook useEffect has a missing dependency: 'navigate'. Either include it or remove the dependency array.eslint[react-hooks/exhaustive-deps](https://github.com/facebook/react/issues/14920) 에러가 잡히고 있네요..!
|
|
||
| const [email, setEmail] = useState(''); | ||
| const [password, setPassword] = useState(''); | ||
| const [isInputError, setInputError] = useState(false); |
There was a problem hiding this comment.
일반적으로는 isInputError 이라면
setIsInputError 가 컨벤션에 맞을 것 같습니다.
| const [existingFiles, setExistingFiles] = useState<string[]>([]); | ||
| const [isModalOpen, setIsModalOpen] = useState(false); | ||
|
|
||
| const categoryOptions = ['연장 근무', '휴일 근무', '무급 휴가', '기타']; |
| const [isModalOpen, setIsModalOpen] = useState(false); | ||
| const [userData, setUserData] = useState<UserType>(); | ||
|
|
||
| const defaultImg = userDefaultSvg; |
| const dispatch = useAppDispatch(); | ||
|
|
||
| const handleModalLogout = async () => { | ||
| await dispatch(fetchSignOut()).then((state) => { |
There was a problem hiding this comment.
await과 then이 혼용되고 있는데
await이 의미 없는 형태가 된 것 같습니다.
There was a problem hiding this comment.
await dispatch(fetchSignOut()) 이 코드만 사용하고 response 결과에 대해서 성공(fulfilled)일때 처리를 하고싶었는데, const result = await dispatch(fetchSignOut()) 이렇게 호출하면 result에서 requestStatus 에 대한 값을 찾지 못해서 이런 코드가 작성되었습니다..
async await를 삭제하고 그대로 사용할지
then을 지우고 성공결과를 체크할지 선택이 필요하겠군요!
| const formattedValue = value | ||
| .replace(/[^0-9]/g, '') | ||
| .slice(0, 11) | ||
| .replace(/(\d{3})(\d{4})(\d{4})/, '$1-$2-$3'); |
There was a problem hiding this comment.
함수명에서 핸드폰 번호 입력을 위함을 유추할 수 있지만
정규 표현식이 어떤 처리를 위함인지 변수로 관리해도 좋을 것 같습니다.
| Icon, | ||
| iconPosition = 'right', | ||
| backgroundButton = false, | ||
| type = 'submit', |
6 participants
🚀 풀 리퀘스트 제안
코드리뷰용 PR입니다
📋 작업 내용
수정한 내용이나 추가한 기능에 대해 자세히 설명해 주세요.
🔧 변경 사항
주요 변경 사항을 요약해 주세요.
📸 스크린샷 (선택 사항)
수정된 화면 또는 기능을 시연할 수 있는 스크린샷을 첨부해 주세요.
📄 기타
추가적으로 전달하고 싶은 내용이나 특별한 요구 사항이 있으면 작성해 주세요.