-
Notifications
You must be signed in to change notification settings - Fork 3
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
[Feat/#84] 회원/비회원 예매 페이지 추가 #105
Conversation
…to feat/#84/MemberBookPage ; Please enter a commit message to explain why this merge is necessary, ; especially if it merges an updated upstream into a topic branch. ; ; Lines starting with ';' will be ignored, and an empty message aborts ; the commit.
…to feat/#84/MemberBookPage ; Conflicts: ; src/pages/book/Book.tsx
PR 작성하느라 고생 많았어요!! 라벨 잘 지정되었는지 확인 한 번 해 주기 🫶 |
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.
로직 상 문제는 없어 보여서 어푸하겠습니다!!!!! 여유로우시다면 styled 파일 분리하면 좋을 것 같아욥~ 굿 근데 진짜 개쩔어욥
src/pages/book/Book.tsx
Outdated
|
||
export default Book; | ||
|
||
const ContentWrapper = styled.div` |
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.
p3) 혹시 styled 파일 일부러 분리 안 하신 걸까요?? 컨벤션이랑 안 맞는 것 같는 것 같아 코드리뷰 남깁니다!
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.
네,, ㅠㅠ 너무 파일이 많아지는 것 같아서,,, 흠,,,
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.
반영했습니다!
|
||
export default RadioButton; | ||
|
||
const Label = styled.label<{ checked: boolean; $isSoldOut: boolean }>` |
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.
p5) 여기두 styled 파일 분리 안 됐어욥 ㅠ.ㅠ.ㅠ.ㅠ.ㅠ.ㅠ
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.
네,,, 감사해요
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.
반영했습니다!
// navigate 할 때 state로 넘기기 ? | ||
const isNonMember = true; | ||
|
||
const [detail, setDetail] = useState(BOOK_DETAIL_INFO); |
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.
p5) 여기 setDetail이 사용되지 않고 있는 것 같은데 나중에 API 연결할 때를 위해서 useState에 넣어두신 걸까요?? 예매 페이지에서 detail을 다시 set 할 경우가 없는 것 같아서요!!
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.
마자욥!
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.
레전드 개발자 정도영.
{/* TODO: eye input으로 변경 */} | ||
<TextField | ||
name="password" | ||
type="input" |
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.
p3) type= "password"
만 바꿔주세용
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.
역시 레개도 👍
모르는 것도 있어서 찾아보면서 리뷰했는데 덕분에 매번 많이 배웁니다 ㅎㅎ
코멘트 한번만 확인해주세요 ~! 고생하셨습니다 !!!
import styled, { keyframes } from "styled-components"; | ||
|
||
export const BottomSheetWrapper = styled.section` | ||
position: fixed; | ||
const bottomSheetUp = keyframes` | ||
0% { transform: translateY(100%); } | ||
100% { transform: translateY(0); } | ||
`; |
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.
p5) css 에서 @Keyframe 으로 정의하던 애니메이션을 styled-components에서는 keyframe 이라는 템플릿 리터럴 함수를 사용해서 정의하는군요! 하나 배워 갑니다 !!
return ( | ||
<S.BottomSheetWrapper onClick={(e) => e.stopPropagation()}> | ||
<S.BottomSheetWrapper $isOpen={isOpen} onClick={(e) => e.stopPropagation()}> |
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.
p5) 혹시 클릭 이벤트 자체를 상위 요소로 전파를 막은 이유가 무엇인가요?
부모 요소에서 클릭 이벤트를 인식하면 안되는 이유라도 있는건가요??
background-color: rgb(0 0 0 / 50%); | ||
visibility: ${({ $isOpen }) => ($isOpen ? "visible" : "hidden")}; | ||
opacity: ${({ $isOpen }) => ($isOpen ? 1 : 0)}; | ||
|
||
transition: | ||
opacity 250ms ease-in-out, | ||
visibility 250ms ease-in-out; |
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.
p5) visibility 라는 속성을 처음 알았는데 굉장히 유용하네요! 눈에는 안 보여도 공간을 차지할 수 있다는 점이 활용도가 높은 것 같아요. display: block | none 만 사용했는데, 나중에 저도 활용해볼게요 !
transition으로 부드러운 css 효과 지정한다음 부드럽게 사라지게 만든 것도 좋습니다~ 센스 굿굿 👍
useEffect(() => { | ||
if (isOpen) { | ||
document.body.style.overflow = "hidden"; | ||
} else { | ||
document.body.style.overflow = "auto"; | ||
} | ||
return () => { | ||
document.body.style.overflow = "auto"; | ||
}; | ||
}, [isOpen]); |
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.
p5) 저도 예전에 스크롤 방지하는 코드를 작성했어야 했는데, 도영님 코드 보고 궁금해서 좀 더 찾아봤어요!
다른 요소들은 그 크기를 넘어서는 순간 overflow가 되는거고, 스크롤바가 생기는 것이 기본인데
body의 경우 뷰포트를 넘어서면 overflow로 인식되고 스크롤바가 생긴다고 하더라구요!
그 지식을 활용해서 애초에 document.body.style.overflow = "hidden" 으로 만들어서 스크롤이 생기지 않게 만들고,
스크롤 자체를 방지한다는 아이디어 너무 좋은 것 같아요. 저번 합세 떄는 무식하게 body의 크기를 제한하고 overflow: "hidden" 으로 설정했는데 정말 코드 깔끔하고 좋은 것 같습니다 ! 많이 배워가요 😊
|
||
// TODO: 회원/비회원 여부 | ||
// navigate 할 때 state로 넘기기 ? | ||
const isNonMember = true; |
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.
p5) 방금 질문하고 답변 들어서 잘 이해갔는데 정리할 겸 코멘트 남겨둡니다!
예매하기 페이지로 넘어오기 전에 있던 페이지에서, navigate 함수를 통해 state를 prop으로 넘겨주고,
이를 예매하기 페이지에서 useLocation을 통해 받아온다는 말씀이신거죠? 그리고 그렇게 받아온 prop에 따라 비회원/회원 예매의 뷰를 다르게 가져가게 되는 거고!
이런 코드로 navigate 할 때, url에 데이터를 노출하지 않으면서 전달할 수 있다는 것도 덕분에 알게 되었습니다 !
const goToBook = () => {
navigate('/details', { state: { isNonMember } });
};
그리고 book 에서는
const location = useLocation();
const { isNonMember } = location.state || {};
와 같은 방식으로 필요한 정보를 받아오고.. 어떤 식으로 작동하는지 전체적으로 이해할 수 있게 되었습니다!
감사합니다 😊👍
const [bookerInfo, setBookerInfo] = useState({ | ||
bookerName: "", | ||
bookerPhoneNumber: "", | ||
birthDate: "", | ||
}); | ||
const [easyPassword, setEasyPassword] = useState({ | ||
password: "", | ||
passwordCheck: "", | ||
}); |
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.
p5) useState를 여러개를 쓰지 않고 연관되어 있는 state들을 모아 하나의 객체로 state를 관리하고,
입력 필드의 name에 배정될 이름을 그대로 key 값으로 지정해놓는 센스 좋네요 !!
덕분에 코드 가독성이 한층 좋아지는 것 같아요 ㅎㅎ
const onChangeBookerInfo = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
const { name, value } = e.target; | ||
|
||
setBookerInfo((prev) => ({ ...prev, [name]: value })); | ||
}; |
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.
p5) 위에서 언급하긴 했지만, 이런 식으로 key값을 input 요소의 name과 동일하게 지정해놓으니까 다음과 같이 코드를 깔끔하게 작성할 수 있는게 좋네요!
onChangeBookerInfo 함수를 input 요소에 onChange 이벤트 핸들러로 연결해놓고, value값을 갱신해서 state로 관리하는 것 좋은 것 같습니다. 굿굿 ~!
let formData = { | ||
scheduleId: performanceId, | ||
selectedValue, | ||
purchaseTicketCount: round, | ||
totalPaymentAmount: detail.ticketPrice * round, | ||
} as FormData; |
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.
p5) let formData : FormData 으로 타입을 설정하는게 아니라, as FormData로 타입 단언을 한 이유가 궁금합니다 !!
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.
아 제가 bookerInfo로 받아오는 타입을
bookerName?
bookerPhoneNumber? 이렇게 옵셔널로 작성한 줄 알았는데, 아니였네요!
옵셔널로 작성한 경우에는 기존의 formData 대입을 할 때, booker와 관련된 정보가 없어서 as FormData로 해야해요!
Quality Gate passedIssues Measures |
📌 관련 이슈번호
🎟️ PR 유형
어떤 변경 사항이 있나요?
✅ Key Changes
📢 To Reviewers
📸 스크린샷
Screen.Recording.2024-07-12.at.1.14.12.AM.mov
🔗 참고 자료