Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Choi-Jiwon-38
left a comment
Choi-Jiwon-38
left a comment
There was a problem hiding this comment.
@tnals0924
코드로만 확인한 내용들이어서 PR 코멘트 반영 중에 동작에 문제 생길 시에는 말씀해주세요! 전부 반영하지 않아도 괜찮은 내용들입니다.
| <input | ||
| className="w-full border-b-2 border-gray-secondary pb-1 pl-1 pt-1 text-start text-body-1-normal_medi placeholder-gray-secondary placeholder:text-body-1-normal_medi focus:outline-none" | ||
| type="text" | ||
| placeholder="학번" | ||
| ref={studentIdRef} | ||
| /> | ||
| <input | ||
| className="w-full border-b-2 border-gray-secondary pb-1 pl-1 pt-1 text-start text-body-1-normal_medi placeholder-gray-secondary placeholder:text-body-1-normal_medi focus:outline-none" | ||
| type="password" | ||
| placeholder="비밀번호" | ||
| ref={passwordRef} | ||
| /> |
There was a problem hiding this comment.
useState가 상태가 바뀔 때마다 뷰를 다시 그린다고 알고 있어서 useRef를 사용했는데 프론트 지식이 많이 없어서 맞는지는 잘 모르겠네요.
@tnals0924
현재 학번이랑 비밀번호를 handleAdminLogin이 실행될 때만 사용하기 때문에 별도 상태값으로 관리하지 않고 ref 를 통해서 값 추적해서 사용하는 거 좋은 거 같습니다.
말해주신 것처럼 state 값이 바뀔 때마다 re-render 되기 때문에 이 경우에는 ref로 직접 접근하는 것이 깔끔해보입니다.
src/utils/loginHandler.ts
Outdated
There was a problem hiding this comment.
@tnals0924
이 부분은 구조분해할당을 이용해서도 축약 가능해보입니다.
const payloadObject = JSON.parse(decodedPayload)
const { role: tokenRole, name: tokenName, sub: tokenId } = payloadObject; 
There was a problem hiding this comment.
우앙 너무 좋은 방법인 것 같습니다!! 사실 제가 작성한 코드였는데... 구조분해할당 매번 사용해야지 하면서도 종종 빼먹더라고요 🥲 저도 구조분해할당을 사용하는 게 더 좋은 방법인 것 같아요!
| const errorResponse = error.response?.data; | ||
|
|
||
| if (!errorResponse) { | ||
| alert('관리자 로그인 중 오류가 발생했습니다!'); | ||
| return; | ||
| } | ||
|
|
||
| alert(errorResponse.message); |
There was a problem hiding this comment.
@tnals0924
오 조기 반환도 좋은 거 같습니다 👍
삼항연산자를 쓰면 아래 처럼도 작성할 수 있을 거 같아요.
alert(errorResponse ? errorResponse.message : '관리자 로그인 중 오류가 발생했습니다!')
| } catch (error) { | ||
| if (axios.isAxiosError(error)) { | ||
| const errorResponse = error.response?.data; | ||
|
|
||
| if (!errorResponse) { | ||
| alert('관리자 로그인 중 오류가 발생했습니다!'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
p4) 개인 선호도... 에 따른 정도인 수준일것 같긴 한데 저는 에러 처리까지 api 요청 함수를 생성할 때 같이 처리하는 편이거든요! 개인적으로 페이지 단에 axios를 import하는 걸 선호하지 않아서...
개인적으로 api 관련 오류는 api 함수에서 처리하는 게 더 가독성이 좋다고 생각하기도 하고, 나중에 추가로 에러핸들링이 필요할 때 인터셉터 등을 처리하기도 더 용이하다고 생각해요! 애초에 제가 privateAxiosInstance를 예쁘게 짰으면 해결될 일이긴 한데... 🥲 나중에 리팩토링 할 때 함께 수정하는 것도 좋은 방식인 것 같습니다!
There was a problem hiding this comment.
넵 확인했습니다~ 해당 내용 나중에 리팩토링할 때 반영하면 좋을 거 같아요.
src/app/desktop/login/page.tsx
Outdated
There was a problem hiding this comment.
p3) 개발적 이슈라기보단 디자인 이슈지만... 생각해보니까 모바일은 구글 로그인인데 어드민은 학번 로그인일 필요가 있었을까요? 특별한 이유가 있는 게 아니라면 통일하는 것도 좋을 것 같아요!
There was a problem hiding this comment.
기존 디자인을 그대로 사용하려고 했던 건데 통일하는 것도 좋을 것 같네요. 일단 이대로 구현하고, 추후에 통일하는 걸로 해도 될 거 같습니다~
There was a problem hiding this comment.
아 까먹고 있었는데 구글 로그인으로 통일할 수 없는 이유가 있었어요! 현재 프론트에서 백엔드 url로 리다이렉팅하는 식으로 구글 로그인을 구현하고 있는데, 이렇게 되면 백엔드에서는 모바일에서 리다이렉팅이 된 건지, 데스크탑에서 리다이렉팅이 된 건지 알기가 힘듭니다. 그래서 프론트 url로 다시 리다이렉팅을 할 때도 그에 대한 정보를 제공해줄 수가 없어서 로그인 방식을 분리하였습니다.
There was a problem hiding this comment.
로그인 페이지에서 어느 페이지에서 왔는지 url을 가지고 있다가 callback 페이지에 넘겨줘도 되고(로컬 스토리지로 해결 가능합니다), 사용자가 모바일에서 접근했는지 PC에서 접근했는지 확인도 가능합니다! 후자가 깔끔해보이긴 하는데 이러면 사용자가 모바일로 어드민 페이지에 로그인해도 다시 모바일로 리다이렉트 되는 이슈가 있어서... 이야기 나눠봐야 할 것 같긴 해요! 아무튼 전달하고 싶었던 의견은 모바일인지 데스크탑인지는 구분이 가능하니 이슈가 그거 하나 뿐이라면 통일하는 것도 좋은 방법일 것 같습니다! 아마 찾아보면 로컬스토리지 외에도 다른 방법이 있을 것 같긴 한데... 일단 지금 떠오르는 방법은 이거 두 개 뿐이네요!
There was a problem hiding this comment.
p5) util로 뺀 거 너무너무 좋습니다!! 안 그래도 콜백 처리할 때 데스크탑까지 분기 처리해야 하나 고민이 있었는데, 지금처럼 데탑은 학번이고 모바일은 구글로 유지할 거라면 로그인핸들러만 공통으로 빼는 것도 좋은 방식이네요!! 공통으로 뺀 거 너무너무 좋습니당 최고!! 🫶
3 participants
📌 관련 이슈번호
🎟️ PR 유형
어떤 변경 사항이 있나요?
Check List
✅ Key Changes
📢 To Reviewers
📸 스크린샷
🔗 참고 자료