Add lecture/#77 #78
Add lecture/#77 #78
Conversation
| @@ -67,3 +67,35 @@ export const fetchDeleteLecture = async (lectureId: number) => { | |||
| isSuccess: true, | |||
| }; | |||
| }; | |||
|
|
|||
| export const fetchAddTakenLecture = async (lectureId: number) => { | |||
There was a problem hiding this comment.
fetchAddTakenLecture addTakenLecture작업을 진행하는 해당 함수의 네이밍에 fetch를 사용하신 이유가 궁급합니다 !
There was a problem hiding this comment.
네이밍을 그렇게 지은 이유는 명확성과 통일성 때문이었습니다
- 과목 삭제 api 함수를처음 작업했을 땐 delete taken lecture 함수가 따로 존재헀어서 fetch delete taken lecture 라고 지었습니다
- 그래서 과목 추가 api 함수도 통일성을 위해 fetch 네이밍을 사용했습니다
- fetch 라는 게 들어가면 데이터를 받아온다는 게 더 명확해진다고 생각했습니다
하지만 언니 질문에 대한 답변을 적다보니, 생각이 바뀌었습니다
command 파일에서 데이터 상태를 바꾸는 작업이니까 fetch 가 들어간다고 대단히 명확하다고 느껴지지 않습니다.
또한 통일성을 따지기엔 taken-lecture.command 파일에 있는 api 함수들 모두 fetch 네이밍을 사용하지 않았습니다
그래서 fetch 를 제거하는 네이밍으로 수정했습니다
좋은 의견 주셔서 감사합니다:)
| const isLectureSearchOpen = useAtomValue(updateDialogAtom); | ||
| const setTakenLectures = useSetAtom(takenLectureAtom); | ||
|
|
||
| useEffect(() => { | ||
| setTakenLectures(initialValue); | ||
| }, [isLectureSearchOpen]); | ||
|
|
There was a problem hiding this comment.
- 리뷰에서 언급하셨듯이, hydrate는 SSR로 생성된 정적 컴포넌트를 사용자 인터랙션 가능한 동적 컴포넌트로 바꾸는 과정이므로, 초기 렌더링 후에만 실행되는 것이 맞습니다.
- 그러므로, 컴포넌트의 역할은 하이드레이션만을 충실하게 처리하는 것이 좋을 것 같아요. 이유는 초기화 로직을 수정하려 할 때, 이 컴포넌트에 위치하고 있다고 생각하기 어려울 것 같기 때문입니다.
- 구조를 살펴보니, TakenLecture 컴포넌트 아래에 초기화를 담당하는 컴포넌트를 선언적으로 만들어 처리할 수 있을 것 같은데, 이 방안에 대해 어떻게 생각하시나요?
There was a problem hiding this comment.
말씀해주신 부분에 동의합니다 taken lecture atom hydrator인데 hydration 외에 다른 기능을 하고 있네요!
taken lectures 를 업데이트 시켜주는 컴포넌트를 추가로 구현해서 분리시켰습니다
네이밍은 초기화 보다 업데이트가 문맥상 적절한 것 같아서 네이밍은 업데이트로 가져갔습니다!
There was a problem hiding this comment.
- 제가 생각한 구조와 완전히 같습니다. 좋네요!
| disabledInfo = { | ||
| value: false, // disabled의 값 | ||
| control: false, // disabled 를 control 하는지 (현재 form에서는 과목 추가에서만 disabled를 control) | ||
| }, | ||
| ...props | ||
| }: FormSubmitButtonProps) { | ||
| const { formId } = useContext(FormContext); | ||
| const { formId, isSuccess } = useContext(FormContext); | ||
| const { pending } = useFormStatus(); | ||
| const disabledValue = disabledInfo.value ? true : disabledInfo.control && isSuccess ? true : false; |
There was a problem hiding this comment.
[comment]
- 'control' 속성의 역할이 무엇인지 잘 이해가 되지 않습니다. 혹시 설명을 부탁드려도 될까요?
- 추가로, 'disable' 속성만 받고, 'disable' 여부를 계산하는 로직을 외부로 이동시킬 수 있는 없는 상황인지 궁금합니다.
There was a problem hiding this comment.
추가 버튼이 disabled 되는 경우는 두 가지 있습니다.
- 이미 추가한 과목 ( 원래 taken lectures에 있는 과목 )
- 추가 버튼을 클릭해서 추가한 과목
- 이미 추가한 과목 같은 경우에는 처음부터 formSubmitButton을 disabled 시켜주면 되기에 disabled 속성만 받으면 됩니다.
- 추가 버튼을 클릭해서 추가된 과목 같은 경우에는 isSuccess가 true가 되면 button을 disabled 시켜줘야합니다.
즉, 처음부터 버튼을 disabled 처리하거나, isSuccess가 true일때 button을 disabled 시켜줍니다
추가 기능만 놓고 봤을 땐 아래와 같이 disabled 속성이 간단히 처리될 수 있습니다
function Button ({disabled}:{disabled:boolean}){
const { formId, isSuccess } = useContext(FormContext);
return <Button disabled={disabled || isSuccess} ... />}
하지만 추가 기능 말고 회원가입, 로그인에서도 form submit button을 사용하고 있기에 isSuccess가 true라고 button을 무작정 disabled 시켜버리면 다른 컴포넌트에서 예상치 못하게 동작을 할 수 있을 것 같아 control 속성과 disabled 여부를 계산 로직을 추가한 것입니다.
control 속성은 isSuccess가 true일 때 button을 disabled 시켜줄 것인지 여부를 나타내는 속성이라고 생각하시면 될 것 같습니다
There was a problem hiding this comment.
저도 해당 코드가 다른 사람이 느끼기에 어려울 거라는 점을 인지하고 있고 개선이 필요한 부분이라고 느낍니다!
isSuccess라는 속성이 필요한데, isSuccess 속성은 FormContext로 부터 얻은 값이기에 외부에서 처리하는 것은 어려울 것 같습니다
There was a problem hiding this comment.
제가 하나 여쭤보고 싶은건, 회원가입과 로그인에서도 isSuccess가 true일 때 button을 disabled 시키는 방향은 어떠신가요!?
코드의 복잡성을 해소하고 싶어서 말씀드린 것도 있지만, 회원가입과 로그인에서 성공했을 때 button을 disabled 시키는 게 UX 적으로도 괜찮을 것 같아서요..!
만약 이게 안된다면 다른 방안을 조금 더 고민해보겠습니다:)
There was a problem hiding this comment.
- 제안하신 방안을 적용하면 문제는 해결되고 기술적으로도 가능하지만, disable을 원하지 않는 유스케이스도 있을 수 있다는 생각이 듭니다.
- 지금 바로 생각나는 것은, form 외부에서 disable을 위한 상태를 가지고 있고, onSuccess 콜백으로 성공 시 disable하도록 변경하는 방안이 있습니다. 이 방안에 대해 어떻게 생각하시나요?
There was a problem hiding this comment.
너무 좋은 방식 같네요 이렇게 되면 form context에 isSuccess 속성도 필요가 없어서 더 깔끔한 코드가 된 것 같습니다
감사합니다:)
74d05c6
There was a problem hiding this comment.
- 확인했습니다. 도움이 되었다니 다행입니다 :)
📌 작업 내용
🤔 고민 했던 부분
공통 컴포넌트 form을 addTakenLectureButton에서도 사용할 수 있도록 몇몇 변경이 있었습니다
addTakenLectureButton에서는 disabled 되는 경우가 두 가지 있습니다
처음부터 isTakenLecture이 true여서 disabled 되는 경우, 추가 버튼을 클릭하여 disabled 되는 경우 (isSuccess가 true일때 )
그래서 disabledInfo를 통해 상황에 맞게 disabled 시켰습니다
addTakenLectureButton에는 form 내부의 style 속성이 필요가 없습니다. 처음에는 style을 className을 통해 받는 걸로 변경할까 했는데 className이 많지 않은 점, 불필요한 style 속성이 여러 태그에 있어서 props가 여러 개 필요한 점 때문에 className을 제거하고 form을
사용하는 곳에서 필요한 스타일을 추가하면 어떨까 싶은데 이에 대해 팀원분들 의견이 궁금합니다!
useAtomHydrator에서 drawer 상태에 따라 state를 업데이트 시켜줬습니다.
추가 버튼을 클릭하고 drawer를 제거했을 때 추가된 과목들이 노출되어있어야 합니다.
하지만 useHydrateAtoms 는 초기 렌더링 시에만 실행이 되는 것 같았고 useEffect 코드를 통해 업데이트 시켜줬습니다.