-
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/#115] 공연진 정보 등록 뷰 구현 #127
Conversation
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.
전체적으로 코드가 길긴 한데 관리할 상태가 많아서 그런 거 같구,,, 제가 감자라서 그런지 크게 고칠 부분은 없어 보입니다!!! 제가 생각하기엔 나중에 여유로울 때 리팩토링 해도 될 것 같아요~~ 일단 어푸하겟슴미다
@@ -2,18 +2,18 @@ import { IconTextfiedlDelete } from "@assets/svgs"; | |||
import { Generators } from "@styles/generator"; | |||
import styled from "styled-components"; | |||
|
|||
export const TextFieldLayout = styled.section<{ narrow?: false | true }>` | |||
export const TextFieldLayout = styled.section<{ $narrow: boolean | undefined }>` |
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) 우리를 괴롭게 했던 그 narrow,,, 결국 성공하셨네요
const RoleLayout = ({ list, updateList, title }: RoleLayoutProps) => { | ||
const [makerList, setMakerList] = useState<Role[]>( | ||
list.map((item, index) => ({ | ||
id: index, // ID 생성 |
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.
p4) 아마 알고 계시겠지만,,, index는 변경 가능성이 있어 id값으로 주지 않는 게 좋다고 알고 있어서요!! id를
id: index, // ID 생성 | |
id: `${castName}-${index}` |
이런 식으로 변경하는 건 어떨까요?
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.
id 타입이 숫자라서 제안해주신 코드로 변경하기는 어려울 것 같습니다...🥹 혹시 더 좋은 방법이 있으시다면 제안해주세요..!
setMakerList((prev) => [ | ||
...prev, | ||
{ | ||
id: Date.now(), |
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) id를 Date.now()
로 하신 이유가 무엇인가요?? 단지 궁금해서요!
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.
maker마다 매칭해서 삭제해주려면 아이디가 필요한데, 공연 등록 api 명세서 보면 maker에는 아이디가 없어서 클라에서만 사용하는 아이디라면 중복되지 않는 값을 새로 가져오고 작성이 완료되면 아이디를 버리면 되겠다고 생각했습니다! 그래서 의미없이 넣어도 되지만 중복은 되지 않도록 아이디를 주기 위해서 사용했습니다.
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.
사실 이걸로 공연진 등록 뷰 2개를 동시에 확인했어서 딱히 여기서 할 말은 없네요!
코드 작성 고생하셨어요 ~~
src/pages/register/RegisterMaker.tsx
Outdated
useEffect(() => { | ||
const allCastFieldsFilled = castList.every( | ||
(cast) => cast.castName && cast.castRole && cast.castPhoto | ||
); | ||
|
||
const allStaffFieldsFilled = staffList.every( | ||
(staff) => staff.staffName && staff.staffRole && staff.staffPhoto | ||
); | ||
|
||
setIsButtonDisabled( | ||
!( | ||
(castList.length === 1 && | ||
!castList[0]?.castName && | ||
!castList[0]?.castRole && | ||
!castList[0]?.castPhoto) || | ||
allCastFieldsFilled | ||
) || | ||
!( | ||
(castList.length === 1 && | ||
!staffList[0]?.staffName && | ||
!staffList[0]?.staffRole && | ||
!staffList[0]?.staffPhoto) || | ||
allStaffFieldsFilled | ||
) | ||
); | ||
}, [castList, staffList]); |
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) 이미 알고 있다니까 할 말은 딱히 없지만 그냥 한번 더 언급해요~
이미지 넣었다가 삭제하면 버튼이 예상치 못하게 비활성화되는 문제만 잘 기억해주세요!
(아마 step1의 경우에는 버튼 활성화 되는 문제점, step 2에서는 버튼 비활성화되는 문제점이긴 합니다)
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.
일단 step1 버튼 활성화 문제는 #109 여기에 커밋해서 푸시올렸습니다! 확인 부탁드려요!
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.
step2도 바녕완~
src/pages/register/Register.tsx
Outdated
if (registerStep === 2) { | ||
return ( | ||
<RegisterMaker | ||
castList={castList} | ||
staffList={staffList} | ||
handleRegisterStep={handleRegisterStep} | ||
updateGigInfo={updateGigInfo} | ||
/> | ||
); | ||
} |
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) else if 사용하는 거 어때요? 어차피 둘 중 하나의 상황이니까, else if를 사용하면 조건문을 검사할 일이 없어서 아주 조금 더 효율적이라는 얘기를 들어서 제안 드립니다 😊
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.
짜피 같은 상태를 값 크기 비교하는 조건문이고 return문으로 종료되니까 그냥 if 써도 상관없다고 생각했는데 코드 가독성측면에서도 else if 쓰는 것이 더 좋을 것 같네요! 수정하겠습니다.
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.
ACK
Quality Gate passedIssues Measures |
📌 관련 이슈번호
🎟️ PR 유형
어떤 변경 사항이 있나요?
✅ Key Changes
📢 To Reviewers
📸 스크린샷
https://www.notion.so/jiwoothejay/ed75708f68224bda8c86aebc0d6d8f25?pvs=4
그그... 머냐... 사실 캡쳐를 예매조회 깔짝하다가 찍었어요,...ㅎㅋㅋ
🔗 참고 자료