Conversation
There was a problem hiding this comment.
고생하셨습니다~!
다음부터는 PR 올리시기 전에 main도 한 번 반영해주시면 리뷰어가 좀 더 편할 거 같아요.
리뷰하면서 페이지 컴포넌트랑 훅을 같이 봐야했는데, 이 브랜치 기준으로 존재하지 않는 내용이라 에디터에서 바로 볼 수가 없더라고요
(참고용으로 적어둔 코멘트고, 지금 main 반영해주실 필요 없습니다-!)
그리고 question/page.tsx에서 calculateResult를 사용해서 결과 계산 + 저장하는 코드는 다른 PR로 올려주시기로 했던 거 맞을까요?
(아직 이슈 생성이 안 된 거 같아 여쭤봅니다)
| const resultTypes: Record<string, number[]> = { | ||
| 야생형: [], | ||
| 교과서형: [], | ||
| 지피티형: [], |
There was a problem hiding this comment.
문득 든 생각인데, 질문에서는 GPT / 결과에서는 지피티로 표기 방식이 다르네요
표현을 하나로 통일하는 게 더 일관된 인상을 줄 수 있을 거 같은데 다른 분들 어떻게 생각하세요?
There was a problem hiding this comment.
저는 결과에서 야생형, 교과서형 같이 유형이름은 한국어로 통일 시키고
유형 소개를 읽으면 지피티가 chat gpt를 의미하는 것을 충분히 알 수 있다고 생각해서 지피티형으로 일단 적었습니다
질문에서는 지피티보다 GPT가 명확한 표현일 것 같은데 만약 질문이랑도 통일을 시키고 싶다면 GPT로 하는 게 좋아 보이네요
There was a problem hiding this comment.
제목끼리 서로 일관성이 있고, 내용에 들어가는 부분은 GPT가 가독성이 있을 것 같아서, 개인적으로는 결과 이름은 제목이라 달라도 괜찮을 것 같아요!
|
main 반영이 어떤건지 모르겠어서 혹시 어떤것을말하는 걸까요??!! 어떻게 하면 된다는걸까요 일단 PR올릴 당시에는 코드가 머지 안되어 있어서 없는 상태로 진행했는데 합쳐서 진행해두겠습니다!! 넵 짧아서 별도 이슈로 올릴까 고민했는데, 짧긴 하지만 의미가 다르다고 생각해서 별도 이슈로 올리겠습니다!! |
피쳐 브랜치에서 작업하시는 동안 main 브랜치에 변경이 생긴다면, 방법은 매번 그렇게 하셔야 한다고 말씀 드린 건 아니였구요, |
|
merge 되기전이라 반영이 안되었던 것 같아요! 이번에 올리면서 반영해두겠습니다! |
…at/result-calculation
- 로직 수정중에 타입 수정된 부분을 함께 수정해서, 타입 수정이 동반되어야 정상작동 하기 때문에 합쳐서 진행했습니다.
namjun12
left a comment
namjun12
left a comment
There was a problem hiding this comment.
결괏값이 평균값으로 계산되는 것 확인했습니다!
고생하셨습니다 :)
nijuy
left a comment
nijuy
left a comment
There was a problem hiding this comment.
고생하셨습니다~
이전 리뷰 중 해결된 코멘트는 resolve 해뒀고, 지피티 GPT 표기 통일에 대한 의견만 남겨주시면 감사하겠습니다 👍
| interface TypeScore { | ||
| sum: number; | ||
| count: number; | ||
| }; |
There was a problem hiding this comment.
TypeScore도 Answers처럼 함수 외부로 빼는 게 더 일관성 있는 위치일 거 같아요 👍
There was a problem hiding this comment.
정리했습니다! interface 위치만 바꿔서 바로 merge 해둘게요!
3 participants
#️⃣ 관련 이슈
💻 주요 변경 사항
💬 To. 리뷰어