Skip to content
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/CK-147] 골룸 생성 페이지 고도화 및 유효성 검사를 구현한다 #114

Merged
merged 20 commits into from
Aug 15, 2023

Conversation

jw-r
Copy link
Collaborator

@jw-r jw-r commented Aug 14, 2023

📌 작업 이슈 번호

CK-147

✨ 작업 내용

  • useFormInput 고도화
  • 골룸 생성 validation
  • 골룸 생성 노드 데이터 맵핑
  • InputField 컴포넌트 생성
  • 인원수 Input 항목 추가

💬 리뷰어에게 남길 멘트

혹여나 잘 이해가 가지 않는 부분이 있다면 언제든 말씀해주세요
화면공유하고 따로 설명드리겠습니다😭

잘 부탁드립니다!

2023-08-14.4.05.00.mov

jw-r added 20 commits August 8, 2023 16:08
@jw-r jw-r added feature 🔅 Improvements or additions to documentation FE 👨‍👨‍👧 FrontEnd labels Aug 14, 2023
@jw-r jw-r requested review from sh981013s and NaveOWO August 14, 2023 07:15
@jw-r jw-r self-assigned this Aug 14, 2023
Copy link
Collaborator

@sh981013s sh981013s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

커밋 기록을 다 보며 확인했는데 계속 훅이 진화하는게 인상적이었어요! ㅋㅋㅋㅋㅋ

너무 고생하셨고, 리뷰 내용만 확인 부탁드려요! vㅔ리 굳입니다!!

  • 영상을 보았을 때 하나의 input validation 만 실패하더라도 form 에 있는 모든 Input 의 validation 이 실패하는 것처럼 보이는 것 같은데 맞나용?

Comment on lines +1 to +7
import { isCurrentOrFutureDate } from '@utils/_common/isCurrentOrFutureDate';

it('과거의 날짜를 입력하면 false를 반환한다', () => {
const isValidateDate = isCurrentOrFutureDate('2022-03-14');

expect(isValidateDate).toBe(false);
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

순수하게 궁금한게 이 하나의 테스트로 테스트 커버리지가 80% 이상이 나왔었나용?? 😮

const [formState, setFormState] = useState<T>(initialState);
const [error, setError] = useState<FormErrorType>();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

초기 값을 넣어주지 않아 undefined 가 나올경우가 있을 것 같네요..! null 값이나 {} 빈 객체를 초기 값으로 할당하는 것은 어떨까요? 🫡

@@ -45,7 +129,6 @@ const useFormInput = <T extends object>(initialState: T) => {
},
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

로대쉬가 그리워지는 로직이군요..! 😀

return (currentValue as any)[part];
}, formState);

const isValid = validate(String(fieldValue));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validateInputValuehandleSubmit 함수에서 유효성 검사 로직이 중복되는 것처럼 보이는데 의도된 상황일까여?? 🙂

@@ -62,6 +145,8 @@ const useFormInput = <T extends object>(initialState: T) => {
formState,
handleInputChange,
resetFormState,
error,
handleSubmit,
};
};

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

배열 핸들링이 코드에 들어가 살짝 복잡해지는 감이 있는데, 아래와 같이 분리해보는건 어떨까여?? 😁

const handleArrayUpdate = (baseName: string, arrayIndex: string, arrayPropName: string) => {
  // 배열 업데이트 로직
};

};

const useFormInput = <T extends object>(
initialState: T,
validations?: ValidationsType
) => {
const [formState, setFormState] = useState<T>(initialState);
const [error, setError] = useState<FormErrorType>();
const [error, setError] = useState<FormErrorType>({});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

엇 아까 리뷰 남겼던 부분이 여기서 이미 해결이 되었군요..!!! 👍👍👍👍

};
});
}
if (isFormValid()) callback();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

callback 보다는 callbackFn 혹은 callbackFunction 같이 조금 더 명시적이면 인자로 받은 콜백 함수겠구나~ 하고 판단하기 조금 더 편하지 않을까 생각이 드네요! 어떻게 생각하시나용

@@ -0,0 +1 @@
export const isEmptyString = (value: string) => value.length === 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

음 개인적으로 유틸 함수의 경우에는 팀원 모두가 자주 쓰게 될 것 같아서 모두 간단한 설명과 사용예시를 담은 주석을 추가하는건 어떨까용?? 😀

@@ -0,0 +1 @@
export const isNumeric = (value: string) => /^[1-9]+\d*$/.test(value);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

주석 부탁려용~~

@@ -0,0 +1,3 @@
export const isValidMaxLength = (value: string, max: number) => {
return value.length <= max;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

주우썩 부탁드려용~~

Comment on lines +30 to +32
<textarea
id={props.name}
name={props.name}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

id는 왜 지정해준건지 궁금해요!!

style?: { [key: string]: string };
};

const InputField = ({ ...props }: InputFieldProps) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여러 props를 받아서 Input을 사용할 수 있게 한거 너무 좋은 것 같아요!! 우디 고민한 티가 많이나고 너무 수고많았어요ㅠㅠ 조금 더 다듬어서 서비스 내 모든 input에 적용해켜봅시당!!
지금 당장은 아니고 리팩토링할 때 했으면 하는 부분들 여기에 정리해놨습니다!
https://www.notion.so/d0b3ea6e9cf44703ab9c3a10bb6e1b96


const useFormInput = <T extends object>(initialState: T) => {
export type FormErrorType = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

타입 선언부 모두 myTypes 폴더로 분리 하는게 좋을 것 같아요~!

const [error, setError] = useState<FormErrorType>({});

const validateInputValue = (name: string, inputValue: string) => {
if (typeof validations?.[name] !== 'function') return true;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

타입 선언부를 보면 validations는 함수를 string인 key와 function인 value를 갖는 객체인 것 같은데, value의 타입이 'function'이 아닌 경우는 왜 구분해준건지 궁금해요!

Comment on lines +7 to +11
export type ValidationReturnType = {
ok: boolean;
message?: string;
updateOnFail?: boolean;
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 타입도 조건부로 선언해야할 것 같아요! ok가 true일 때는 message는 있으면 안되고, ok가 false일 때만 message가 ValidationFunctionType의 반환값에 들어있어야 하는게 아닌가요?!

Comment on lines +102 to +103
Object.keys(validations).forEach((key) => {
const result = validations[key](String(getNestedValue(formState, key)));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

객체의 정확한 타입 추론을 위해서 util 폴더의 invarientType에 있는 유틸함수르 이용해서 object.keys()를 사용해주세요!

Copy link
Collaborator

@NaveOWO NaveOWO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

우디 너무너무 수고햐셨어요~~!!!!! 코멘트 확인해주세요!

@sh981013s sh981013s self-requested a review August 15, 2023 05:20
Copy link
Collaborator

@sh981013s sh981013s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vㅔ리 굳이에용~~

@sh981013s sh981013s merged commit 791dd5b into develop-client Aug 15, 2023
1 check passed
@sh981013s sh981013s deleted the feature/CK-147 branch August 15, 2023 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FE 👨‍👨‍👧 FrontEnd feature 🔅 Improvements or additions to documentation
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

골룸 생성 시 날짜 입력들이 placeholder가 아닌 것 같아요
4 participants