-
Notifications
You must be signed in to change notification settings - Fork 6
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-fe: 지원서 입력 폼 임시 저장 기능 구현 #976
base: fe/develop
Are you sure you want to change the base?
Conversation
1735792227.771169 |
1735792228.639899 |
1735823110.956249 |
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.
작업 고생하셨습니다 러기🙏
임시 저장본 복구에 타이밍에 관해서 이야기해보고 싶은 게 있어 RC 드렸어요.
자세한 내용은 코멘트 확인 부탁드립니다~
const [enableStorage] = useState(() => { | ||
const prevSavedAnswer = window.localStorage.getItem(LOCALSTORAGE_KEY); | ||
if (!prevSavedAnswer) return false; | ||
|
||
const prevSavedAnswerIds = Object.keys(JSON.parse(prevSavedAnswer)); | ||
if (prevSavedAnswerIds.every((id) => questions.some(({ questionId }) => questionId === id))) { | ||
return window.confirm('이전 작성중인 지원서가 있습니다. 이어서 진행하시겠습니까?'); | ||
} | ||
}, [questions]); | ||
return false; | ||
}); |
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.
최초 렌더링 시점에만 로컬 스토리지를 들여다 보도록 콜백함수를 initial state로 넘기셨네요. 요런 기법을 "게으른 초기화"라고 하는 거군요! 두 분 덕분에 또 배워갑니다 🙏
baseInfoHandlers: { | ||
handleName: (e: React.ChangeEvent<HTMLInputElement>) => void; | ||
handleEmail: (e: React.ChangeEvent<HTMLInputElement>) => void; | ||
handlePhone: (e: React.ChangeEvent<HTMLInputElement>) => void; | ||
}; |
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.
업데이트하는 필드의 이름만 다르고 내용은 같은데 함수를 중복해서 내보내는 건 사용하는 쪽에서의 DX를 고려한 것이라 이해하면 될까요?
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.
(e: React.ChangeEvent<HTMLInputElement>) => void;
메서드를 코드분리 하지 않은 것이 DX를 고려한 것인지 여쭙는 것 맞으실까요? 맞습니다!
저는 저 각각의 함수가 다른 타입을 가질 수 있다고 생각해요. 수정이 일어난다면 최소수정원칙을 따르는게 좋을 것이라 생각합니다! handleName의 타입이 변경된다고 해서 handleEmail의 타입이 바뀔 것이란 보장은 없을 것이라 생각하는 것입니다~!
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.
baseinfo에 포함된 Name, Email, Phone에 대한 input field type이 변경될 일이 과연 있을 것인지...는 모르겠습니다만, Provider에 포함된 baseInfoHandlers 메서드에 중복 로직을 감수하고 요렇게 구현하신 의도에 대해서는 이해했습니다. 👍
const [enableStorage] = useState(() => { | ||
if (window.localStorage.getItem(LOCALSTORAGE_KEY)) { | ||
if (window.confirm('이전 작성중인 지원서가 있습니다. 이어서 진행하시겠습니까?')) { | ||
moveTabByParam('지원하기'); | ||
return true; | ||
} | ||
} | ||
return false; | ||
}); |
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.
우선 꼼꼼히 봐주셔서 감사드려용~!
두개의 confirm이 나타나는 것은 StrictMode여서 그렇습니다!
strict 모드에서도 한 번만 나타나게 구현하는 것도 고려해봤습니다.
- Ref로 만들어 보았지만, 문제가 있었는데요,
기존에는, 지원하기 -> 모집공고 -> 지원하기로 돌아온다면, 작성되어있던 값들이 사라지는 상황이었습니다.
이유는 이전 "모집공고 탭"과 "지원하기" 탭의 높이 차이로 인한 스크롤 문제 때문이었는데요.
이 상황에서 Ref를 사용하니, 재 렌더링시(지원하기 -> 모집공고 -> 지원하기)마다 confirm을 물어보는 문제가 있어서 useState를 사용했습니다. - 이 모두를 해결하는 방법으론 Closure를 만드는 것 정도가 있을 것 같은데, 저는 이정도 구현으로도 사용성에 큰 문제가 없다고 생각했어요!
JD를 확인하려는 목적에선 confirm에서 단순히 취소를 누르면 된다고 생각했습니다!
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.
저는 "취소"를 눌렀을 때 기존의 지원서 작성 내용이 초기화되는 현재의 동작 흐름이 자연스럽다고 생각해요. '사용자가 명시적으로 이전 내용을 불러오지 않겠다'는 의사 표현을 한 것이니까요.
다만 지원자가 지원서를 작성하던 도중 화면을 이동하거나 탭을 닫은 뒤, 다시 공고 페이지로 넘어왔을 때 바로 '작성 중인 지원서의 로드' 여부를 물어보는 흐름은 조금 어색한 것 같아요. 공고 URL로 들어왔을 때 먼저 기대되는 내용은 JD라고 생각했거든요. "지원하기" 탭을 눌렀을 때 지원서 로드 여부를 물어보는 방식으로의 구현에 대한 러기의 생각을 여쭤보고 싶어요!
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.
말씀대로 구현하는게 베스트라 생각합니다! 위에서 언급한 것 처럼 Closure를 만드는 방법이 있을텐데, 이 방법으로 재구현 하는게 좋을까요?
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.
러기 고생하셨습니다. 이전에 개발해주신 useLocalStorageState와 더불어 어려운 작업을 손수 해결해 주셔서 감사한 마음입니다. 아울러 리뷰가 많이 늦여진 점에 대해 사과 드립니다.
코드나 로직에 대한 수정 필요사항은 없으나, 렛서와는 다른 의미로 '작성 중인 지원서 로드' 여부를 물어보는 타이밍에 대해 의견을 구하고 싶은 부분이 있어 코멘트를 남겨드렸습니다. 여유 되실 때 살펴주세요. 😄
const [enableStorage] = useState(() => { | ||
const prevSavedAnswer = window.localStorage.getItem(LOCALSTORAGE_KEY); | ||
if (!prevSavedAnswer) return false; | ||
|
||
const prevSavedAnswerIds = Object.keys(JSON.parse(prevSavedAnswer)); | ||
if (prevSavedAnswerIds.every((id) => questions.some(({ questionId }) => questionId === id))) { | ||
return window.confirm('이전 작성중인 지원서가 있습니다. 이어서 진행하시겠습니까?'); | ||
} | ||
}, [questions]); | ||
return false; | ||
}); |
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.
최초 렌더링 시점에만 로컬 스토리지를 들여다 보도록 콜백함수를 initial state로 넘기셨네요. 요런 기법을 "게으른 초기화"라고 하는 거군요! 두 분 덕분에 또 배워갑니다 🙏
const [answers, setAnswers] = useLocalStorageState<AnswerFormData>( | ||
(() => questions.reduce((acc, question) => ({ ...acc, [question.questionId]: [] }), {} as AnswerFormData))(), | ||
{ | ||
key: LOCALSTORAGE_KEY, | ||
enableStorage, | ||
}, | ||
); |
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.
로컬 스토리지에 넣을 응답데이터의 초기값을 IIFE로 넣으셨군요! 저는 데이터 변환 로직을 별개의 함수로 분리하는 것을 선호하긴 합니다만, 다른 곳에 재사용할 가능성이 없는 로직이라면 요런 방법도 괜찮아 보입니다 ㅎㅎ
baseInfoHandlers: { | ||
handleName: (e: React.ChangeEvent<HTMLInputElement>) => void; | ||
handleEmail: (e: React.ChangeEvent<HTMLInputElement>) => void; | ||
handlePhone: (e: React.ChangeEvent<HTMLInputElement>) => void; | ||
}; |
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.
baseinfo에 포함된 Name, Email, Phone에 대한 input field type이 변경될 일이 과연 있을 것인지...는 모르겠습니다만, Provider에 포함된 baseInfoHandlers 메서드에 중복 로직을 감수하고 요렇게 구현하신 의도에 대해서는 이해했습니다. 👍
const [enableStorage] = useState(() => { | ||
if (window.localStorage.getItem(LOCALSTORAGE_KEY)) { | ||
if (window.confirm('이전 작성중인 지원서가 있습니다. 이어서 진행하시겠습니까?')) { | ||
moveTabByParam('지원하기'); | ||
return true; | ||
} | ||
} | ||
return false; | ||
}); |
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.
저는 "취소"를 눌렀을 때 기존의 지원서 작성 내용이 초기화되는 현재의 동작 흐름이 자연스럽다고 생각해요. '사용자가 명시적으로 이전 내용을 불러오지 않겠다'는 의사 표현을 한 것이니까요.
다만 지원자가 지원서를 작성하던 도중 화면을 이동하거나 탭을 닫은 뒤, 다시 공고 페이지로 넘어왔을 때 바로 '작성 중인 지원서의 로드' 여부를 물어보는 흐름은 조금 어색한 것 같아요. 공고 URL로 들어왔을 때 먼저 기대되는 내용은 JD라고 생각했거든요. "지원하기" 탭을 눌렀을 때 지원서 로드 여부를 물어보는 방식으로의 구현에 대한 러기의 생각을 여쭤보고 싶어요!
Original issue description
목적
작업 세부사항
참고 사항
SAVE-AUTO-APPLICNAT
closes #975
참고사항
#978 해당 이슈 먼저 Merge 이후 진행
⚒️ 변경사항
ApplyAnswerContext
추가name
,email
,phone
)와 질문별 응답 데이터를localStorage
를 활용해 저장 및 복원.useAnswers
훅 개선localStorage
와의 상호작용 로직 추가.resetStorage
기능을 통해 제출 시 데이터 초기화 처리.ApplyForm
컴포넌트 리팩토링useForm
은 그대로 사용.onChange
핸들러 연결.RecruitmentPost
컴포넌트의 중복 렌더링 이슈로 인해 동적 임포트를 임시로 제거.endDate
데이터 최신화.💬 논의 사항
동적 임포트 복구 시점
RecruitmentPost
컴포넌트의 중복 렌더링 이슈 해결 후 동적 임포트를 복구해야 할 시점 논의 필요.