-
Notifications
You must be signed in to change notification settings - Fork 44
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
경북대 FE_이효은 6주차 과제 Step1,2 #33
base: hyoeunkh
Are you sure you want to change the base?
Conversation
안녕하세요~ |
feat: 카카오로 로그인하기 버튼 생성 feat: option 선택 기능 구현 style: 코드 포매팅 feat: 주문하기 버튼 클릭 시 새 주문 생성 feat: 주문 목록 페이지 구현 feat: baseURL 환경변수 설정 feat: post wish api body 변경 feat: baseURL 환경변수 설정 style: api 협의 후 헤더 변경 feat: 주문 내역 mock 설정 feat: 주문 내역 ui 맞게 조정 refactor: 코드 포매팅 feat: 배포 feat: login, register 수정 test: @testing-library/react 최신버전 test: @testing-library/react 최신버전 test: test 잠시 주석 처리 feat: 로그인, 회원가입 api 수정
충돌 해결했습니다! |
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.
고생하셨습니다~~
리뷰할 내용이 많지 않네요! 👍
테스트는... 제가 지금 돌려볼 수 있는 환경이 아니라서 리뷰가 좀 어렵네요 ㅜㅜ
이외에 대부분 잘 작성해 주신 것 같아요!
좀 더 생각해볼 부분이라면 코드가 명령형인 경우가 조금 보이는데, 선언적으로 작성해볼 수 없을지 고민해 보시면 좋을 것 같습니다.
예를 들면, 데이터가 loading인지, error인지, !data인지, length가 0인지 등을 if문을 사용한 명령형 코드로 작성해 주셨는데, 이 부분을 선언적으로 처리할 수 있는 방법은 없을까요?
src/pages/Login/index.tsx
Outdated
const redirectUrl = queryParams.get('redirect') ?? `${window.location.origin}/`; | ||
return window.location.replace(redirectUrl); |
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.
react-router-dom 대신 window.location을 사용하는 부분이 많은데 혹시 이유가 있을까요??
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.
제공된 코드 사용한 거라 강사님께서 작성하신 코드입니다! react-router-dom 사용하는게 더 좋은 방법일까용?
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.
아하 보일러플레이트군요
spa로 동작하게 하려면 react-router-dom을 사용해야하고, window.location은 아예 앱을 다시 실행시킬거라 상황에 맞게 사용하시면 될 것 같아요. 굳이 새로고침이 필요하지 않다면 react-router-dom이 더 좋은 선택지라고 생각해요
// TODO: 추후 서버 API 주소 변경 필요 | ||
export const BASE_URL = process.env.REACT_APP_API; | ||
export const fetchInstance = initInstance({ | ||
baseURL: 'https://api.example.com', | ||
baseURL: process.env.REACT_APP_API, | ||
}); |
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.
바로 위에 상수를 선언했는데 다시 env를 참조하고 있네요...?
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.
수정했습니다 감사합니다!
안녕하세요 멘토님!
백엔드 분들께서 cors 설정을 덜 마친 관계로 api가 잘 적용되었는지 확인은 못했지만, mock data 만들어서 구현해두었습니다.
api 잘 되는지는 step2에서 배포와 함께 마저 하도록 하겠습니다.
감사합니다 :)
++ 실수로 step1 브랜치에 배포 설정 커밋해버렸습니다.. ㅎㅎ
https://react-deploy-2ct.pages.dev/ 배포주소입니다!
++ 갑자기 왜 test에서 action이 막히는지 모르겠습니다. . 일단 주석처리로 해결했습니다!