타임라인 수정 기능 추가 - #215
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 타임라인 이벤트에 대한 수정 기능을 도입하고, 기존 이벤트 추가 모달을 재활용하여 단일 모달로 통합합니다. 또한, 타임라인 카드들의 시각적 일관성과 가독성을 높이기 위한 CSS 개선 사항도 포함되어 있습니다. 이벤트 수정 시 데이터 캐시를 즉시 무효화하여 사용자에게 최신 정보를 제공합니다. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
타임라인 수정 기능 추가를 위한 변경 사항들을 잘 확인했습니다. 전반적으로 EventAddModal을 EventModal로 일반화하여 코드 재사용성을 높인 점과, Sentry를 이용한 에러 로깅 개선이 인상적입니다.
한 가지 제안 사항으로, 수정 모달의 상태 관리를 개선하여 사용자가 모달을 취소했을 때도 상태가 올바르게 초기화되도록 하는 부분을 리뷰 코멘트로 남겼습니다. 이를 통해 더욱 안정적인 코드가 될 것으로 기대합니다.
라이브러리 제약으로 인해 UI 구현에 어려움이 있으셨던 것 같은데, 커스텀 렌더링으로 잘 해결하신 것 같습니다. 수고하셨습니다.
| const { | ||
| open: openEditModal, | ||
| close: closeEditModal, | ||
| component: editModalComponent, | ||
| } = useModal( | ||
| editingEvent ? ( | ||
| <EventModal mode="edit" event={editingEvent} onCancel={() => closeEditModal()} onSubmit={handleEditEvent} /> | ||
| ) : ( | ||
| <></> | ||
| ), | ||
| ); |
There was a problem hiding this comment.
수정 모달을 닫을 때 editingEvent 상태를 초기화하는 로직을 추가하는 것이 좋습니다. 현재는 수정이 성공했을 때만 상태가 초기화되고, 사용자가 '취소'를 누르거나 모달 외부를 클릭하여 닫을 경우에는 상태가 그대로 남아있게 됩니다. 이로 인해 예기치 않은 동작이 발생할 수 있습니다.
useModal 훅의 onClose 옵션을 사용하면 모달이 닫힐 때 항상 특정 동작을 수행하도록 할 수 있어 코드를 더 깔끔하고 안전하게 만들 수 있습니다.
const {
open: openEditModal,
close: closeEditModal,
component: editModalComponent,
} = useModal(
editingEvent ? (
<EventModal mode="edit" event={editingEvent} onCancel={() => closeEditModal()} onSubmit={handleEditEvent} />
) : (
<></>
),
{
onClose: () => setEditingEvent(null),
},
);
There was a problem hiding this comment.
jinhokim98
left a comment
There was a problem hiding this comment.
바로 고쳐주셨군요 감사합니다
수정버튼 저는 안에 들어간게 더 좋은 것 같아요. 이 이벤트 수정이다. 이게 잘 보이는 것 같아요.
가벼운 코멘트 남겨놨습니다
There was a problem hiding this comment.
이번 수정사항은 아니지만 가벼운 제안 하나 드려봅니다~ 지금은 이벤트가 내용이 적지만 내용이 더 많아질 때 이런 유효성 검증이 핸들러 내에 정의되어있으면 찾기 힘들어지더라구요..
예로들어 이벤트 생성, 수정 내 유효성 정책이라는 파일을 만들어 이름 최소 최대 길이, 날짜 제한 규칙 등을 정리해둔 후 사용하면 나중에 봤을 때 찾기 좋더라구요..! 이럴 때 zod가 아주 기가막힌데 이 정도 검증에 사용하기엔 굳이 싶기도 하구요ㅋㅋ
지금은 간단해서 변경하지 않아도 됩니다~
There was a problem hiding this comment.
오호 그렇네요! 유효성 검증 얘기하니 우테코 초반에 로또랑 자동차 경주 미션때 validation 코드 열심히 페어랑 토론하던게 생각나네요ㅋㅋㅋ 나중에 리팩토링 하면 좋을 것 같아요!
| items={timelineItems} | ||
| mode="HORIZONTAL" | ||
| cardHeight={120} | ||
| cardHeight={200} |
There was a problem hiding this comment.
좋은 것 같아요. 고정되어있으면 더 예쁠 것 같아요
dev 배포 스크립트 수정fix: dev 배포 스크립트에 GITHUB_REF 환경변수 전달 이 PR에서 브랜치 전략을 변경하면서 배포 스크립트도 수정을 했는데, 거기에 문제가 있어서 dev CD가 실패하고 있었어요.
문제
수정ssh-action에 참고사항prod 배포(frontend-prod-deploy.yml)는 브랜치명을 main으로 하드코딩하고 있어서 문제 없습니다! |
chosim-dvlpr
left a comment
There was a problem hiding this comment.
고생하셨습니다~~ 리뷰 달았어요! 환경변수 추가 감사합니다😊😊
| username: ${{ secrets.SSH_DEV_USERNAME }} | ||
| key: ${{ secrets.SSH_DEV_KEY }} | ||
| port: ${{ secrets.SSH_DEV_PORT }} | ||
| envs: GITHUB_REF |
| const handleOpenEditModal = (event: OrganizationEventResponse) => { | ||
| setEditingEvent(event); | ||
| openEditModal(); | ||
| }; |
There was a problem hiding this comment.
리액트의 상태 변경 batching 타이밍 이슈가 있어 useEffect로 editingEvent에 event를 넘겨주는 방법은 어떨까요?
There was a problem hiding this comment.
이렇게 하는 방법을 얘기하시는 걸까요?
const handleOpenEditModal = (event: OrganizationEventResponse) => {
setEditingEvent(event);
};
useEffect(() => {
if (editingEvent) {
openEditModal();
}
}, [editingEvent]);
https://ko.react.dev/blog/2022/03/29/react-v18#new-feature-automatic-batching
React 18부터 이벤트 핸들러 내 상태 업데이트는 자동 batching되어 한 번의 리렌더링으로 처리되기 때문에, 현재 코드에서 batching 타이밍 이슈는 발생하지 않을 것 같아요. useEffect를 쓰면 오히려 불필요한 리렌더링이 한번 더 발생하지 않을까 싶어요!
There was a problem hiding this comment.
음.. 저는 다른 생각인데요. setEditingEvent 자체를 상태로 관리하지 않고 모달이 열릴 때 상태를 넣어주면 된다고 생각해요.
이렇게 한다면 가독성도 챙기고 useEffect도 사용하지 않게 될 것 같아요.
There was a problem hiding this comment.
@ShinjungOh
현재는 모달이 하나로만 관리되고 있지만 수정모달/생성모달을 분리하는 건 어떨까요?
복잡성이 높아지는 느낌이라 추상화 레벨을 낮추고 아예 기능별로 분리하는 것도 한 방법이 될 것 같아요!
There was a problem hiding this comment.
이벤트 추가/수정 모달을 별개의 컴포넌트로 분리했어요. 이 코멘트에서 프룬이 질문하신 'event가 옵셔널인 이유'도 리팩토링하면서 필수 prop으로 변경했습니다.
| import {OrganizationEventResponse} from '@type/Group.type'; | ||
|
|
||
| interface EventAddModalProps { | ||
| type EventModal = 'add' | 'edit'; |
There was a problem hiding this comment.
EventModal 타입 이름과 컴포넌트명이 동일해서 타입명을 바꾸는 게 좋을 것 같아요!
There was a problem hiding this comment.
아 타입명 지을 때 신경을 못썼네요.. 수정했어요~!
|
|
||
| interface EventModalProps { | ||
| mode: EventModal; | ||
| event?: OrganizationEventResponse; |
There was a problem hiding this comment.
여기서 event가 옵셔널인 이유가 있을까요??
There was a problem hiding this comment.
추가/수정 모달을 하나의 컴포넌트로 리팩토링하면서 추가 모드에서는 event가 필요 없기 때문에 옵셔널로 선언했어요.
추가 모드에서는 새 이벤트를 생성하는 것이라 기존 이벤트 데이터가 없고,
수정 모드에서만 기존 이벤트 내용을 미리 채워넣기 위해 event를 전달해요!
There was a problem hiding this comment.
request.json 파싱에 오류가 생길 수 있어 eventData도 26번라인의 try-catch문에 넣는 것은 어떨까요?
There was a problem hiding this comment.
chosim-dvlpr
left a comment
There was a problem hiding this comment.
가벼운 리뷰 하나 달았습니다! 고생하셨어요~!!👏
| <div> | ||
| <div className="mb-2 flex justify-between text-base font-bold"> | ||
| <span className="text-grayscale-600">제목</span> | ||
| <span className="text-sm font-normal text-grayscale-300">{title.length}/20</span> |
There was a problem hiding this comment.
[제안]
20, 50과 같은 매직넘버는 위에서도 사용되니 상수로 관리되면 좋을 것 같아요~
There was a problem hiding this comment.
상수화 리팩토링 완료했습니다~!
* fix: 타임라인 카드 하단 텍스트 색상 수정 * feat: 타임라인 이벤트 수정 기능 추가 * refactor: 이벤트 추가/수정 모달 통합 * fix: 타임라인 수정 버튼 위치 정렬 * fix: 타임라인 이벤트 수정 후 캐시 갱신되지 않는 문제 해결 * fix: 수정 모달 닫힐 때 editingEvent 상태 초기화 * fix: dev 배포 스크립트에 GITHUB_REF 환경변수 전달 * refactor: EventModal 타입명과 컴포넌트명 중복 해결 * fix: eventData 파싱을 try-catch 내부로 이동 * refactor: 이벤트 추가 모달/수정 모달 분리 * refactor: 이벤트 모달 매직넘버 상수화

issue
구현 사항
1. 수정 기능 추가
수정 버튼을 추가하고, 버튼을 누르면 수정 모달을 통해 내용을 변경할 수 있도록 했습니다.
이벤트 추가시 입력한 내용이 수정 모달에 프리필(pre-fill)되어있고, 사용자가 이를 수정하는 방식입니다.
수정시 캐시가 무효화되어 바로 변경 사항이 보입니다.
추가/생성 모달을 하나로 합치고 mode로 구분하도록 구현했습니다.
CleanShot.2026-03-19.at.17.25.23.mp4
2. 카드 CSS 수정
텍스트 하단 부분의 컴포넌트가 마지막 줄의 색상을 흐리게 만들던 문제를 해결했습니다.☺️
카드 컴포넌트의 높이를 일괄로 지정했습니다. 원래 텍스트 분량에 맞춰서 조절되도록 했으나.. 텍스트 최대 글자가 50자로 지정되어 있어서 높이를 통일하는게 깔끔한 것 같아유
그 외 색상 리팩토링(CSS hex 값을 colors.ts 기준으로 통일) 등, 자잘한 스타일 변경이 있습니다.
중점적으로 리뷰받고 싶은 부분(선택)
어떤 부분을 중점으로 리뷰했으면 좋겠는지 작성해주세요.
논의하고 싶은 부분(선택)
수정 버튼이 카드 내부에 있는 것이 어색하거나 UIUX적으로 괜찮을까요?
이걸 질문하는 이유: 원래 시안은 스샷처럼 border 밖에 위치하는 것을 목표로했으나..
이 PR의 논의하고 싶은 부분을 보면 이렇게 적어뒀는데요, 라이브러리와 스타일적으로 충돌하는 문제가 있어 지금의 방법을 채택했습니다. 코드 복잡도가 높아져서요🫠
🫡 참고사항