[refactor] 원서 작성 페이지 리펙토링 #167
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
원하시는 부분만 집중적으로 리뷰 받기 위해서, |
조언 감사합니다 main 브랜치의 내용을 develop 브랜치로 merge하는 pr을 생성하였습니다. |
…front-24 into refactor/oneseoWritingPage
| useEffect(() => { | ||
| setTimeout(() => | ||
| ACHIEVEMENT_FIELD_LIST.forEach((field) => { | ||
| achievementList.some((freeGrade) => freeGrade.field === field) | ||
| ? setValue(field, watch(field) || []) | ||
| : setValue(field, null); | ||
| }), | ||
| ); | ||
| }, []); |
There was a problem hiding this comment.
여기에서 setTimeout은 왜 추가하신 건가요?
There was a problem hiding this comment.
step4 필드를 다 채운상태에서 임시저장후 /register?step=4로 새로 접속했을때 점수 계산하기 버튼과 원서 제출 버튼이 활성화가 안되는 경우가 있었습니다 예전에도 개발하면서 이런 문제가 있어서 setTImeout 비동기 함수를 사용해서 해결하였습니다.
제가 원인 분석을 해보았을떄는 setTimeout 안에 있는 코드가 사용되지 않는 achievementX_X 필드를 null로 만드는 코드인데 해당 코드가 step4 useForm 초기값 할당하는 코드보다 먼저 실행되서 사용되지 않는 필드가 undefined 값이 할당되어서 버튼이 활성화가 안되는 문제로 파악 됩니다.
궁금하시다면 원서를 step 4까지 작성후 임시저장을 한다음에 /register?step=4에서 새로고침을 setTimeout이 있을떄 없을떄를 해보면 될것 같습니다.
저번에도 이러한 문제가 있었을때 setTimeout으로 해결했던 pr 입니다 #30
근데 이것과 별게로 setTimeout 2번째 파라미터를 넣지 않았네요 6ddb2da
| const isCandidate = graduationType === GraduationTypeValueEnum.CANDIDATE; | ||
| const isGED = graduationType === GraduationTypeValueEnum.GED; | ||
| const isGraduate = graduationType === GraduationTypeValueEnum.GRADUATE; |
There was a problem hiding this comment.
GraduationTypeValueEnum.CANDIDATE처럼 Enum 값을 여러 번 직접 사용하는 대신, 상위에서 구조 분해 할당하여 사용하는 게 어떨까요?
const { CANDIDATE, GED, GRADUATE } = GraduationTypeValueEnum;
const isCandidate = graduationType === CANDIDATE;
const isGED = graduationType === GED;
const isGraduate = graduationType === GRADUATE;There was a problem hiding this comment.
enum도 구조 분해 할당이 되는지는 몰랐네요
그치만 제 의견을 말씀드리자면 enum이 현재 대문자로 되어있는데 구조분해 할당을 하여 사용한다면 추후에 코드를 볼떄 상수를 사용하고 있다고 생각할수도 있을것 같습니다 그래서 구조 분해 할당을 사용하지 않는 방식이 enum을 사용하고 있다는 사실을 명확하게 나타내는것 같습니다
다른 분들의 의견도 여쭈어봐야 할것 같습니다
There was a problem hiding this comment.
좋은 의견 감사합니다! 구조 분해 할당을 사용하면 코드가 간결해지긴 하지만, 말씀하신 대로 enum이 무엇을 나타내는지 명확하게 인지할 수 있는 점에서는 구조 분해 할당이 다소 혼동을 줄 수 있을 것 같습니다. 특히 대문자 상수와 비교했을 때 enum이라는 사실을 코드만 보고 구별하기 어려울 수 있겠네요.
해당 의견에 대해서 어떻게 생각하시는지 궁금합니다! @yoosion030 @frorong @hyeongrok7874
There was a problem hiding this comment.
좋은 리뷰 주제네요, 코드를 보았을 때 네이밍에 enum이라는 것이 드러나기 때문에, 구조 분해 할당을 사용했을 때 enum이 상수처럼 보인다고 느껴지진 않는 것 같습니다.
저는 구조 분해 할당을 사용하여 가독성을 줄이는 관점 보다는 enum을 사용하는게 맞는 관점인지 먼저 논의해보면 좋을 것 같다고 생각해요. enum을 자주 사용해보진 않아서, 현재 케이스에 enum을 사용하는게 맞는지 의문이 들긴하거든요.
https://xpectation.tistory.com/218
해당 자료 참고하여 enum, const enum, as const 중에 어떠한 것으로 타입을 관리할 것인지 먼저 의논한 뒤에 코드 가독성에 대한 내용을 다루면 좋겠네요.
There was a problem hiding this comment.
글을 읽고 처음 생각했을때는 enum은 양방향 맵핑이 가능하다고 해서 현재 상황에서는 사용하지 않기 떄문에 as const를 사용하면 된다고 생각했었는데 조금 더 찾아보니 숫자형 enum만 양방향 맵핑이 가능해서 기존 코드를 그대로 유지해도 상관 없다고 생각합니다. 그리고 제가 enum을 도입한 이유를 말씀드리자면 기존의 방식은 유니온 타입으로 되어있었는데 값을 할당할때나 값을 비교할때 더 명시적으로 사용할수 있어 도입하게 되었습니다
|
@gaoooon |
넵 좋습니다! 바로 진행하겠습니다 |
개요 💡
원서 작성 페이지를 리펙토링 하였습니다.
작업내용 ⌨️
큰 변경사항은 아래와 같습니다.
기존 문제
(예시. step 1 필드를 모두 다 채워서 step 2 컴포넌트로 넘어가려고 해도 step 2, 3의 필드가 작성되어 있지 않아서 원하는대로 작동이 되지 않습니다)
관련 issue 🤯
생각보다 file changed가 많아졌습니다.. (main에 반영된 사항도 같이 합쳐저서 더 많아진것 같습니다)