Skip to content

feat: record post form 추가 #23

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

Merged
merged 9 commits into from
Sep 1, 2024
Merged

feat: record post form 추가 #23

merged 9 commits into from
Sep 1, 2024

Conversation

d0422
Copy link
Contributor

@d0422 d0422 commented Aug 31, 2024

  • Record post 폼추가
  • mock mutation함수 추가
  • TextField multiline props추가

@d0422 d0422 added the feat 새로운 기능 추가 label Aug 31, 2024
@d0422 d0422 requested a review from HBSPS August 31, 2024 04:21
@d0422 d0422 self-assigned this Aug 31, 2024
Copy link
Contributor

@HBSPS HBSPS left a comment

Choose a reason for hiding this comment

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

수고하셨습니다!

Comment on lines +34 to +36
const validate = () => {
return title.length > 0 && images.length > 0;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

const lengthValidate = (...args: string[]) => {
  for (const arg of args) {
    if (arg.length === 0) return false;
  }
  return true;
}

이런 validate 함수들은 이런 식으로 별도의 함수로 만들어도 좋을 것 같아요!

Copy link
Contributor Author

@d0422 d0422 Aug 31, 2024

Choose a reason for hiding this comment

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

array 여러개를 받아서 확인한다는게, 유효성 확인이라는 관점에서 봤을때, 범용성이 있다고 보기는 조금 어려울 것 같아요(유효성 검증은 array만 하는 것이 아닐 수 있으므로)
같은 부분을 제거하려면 실제 구현부를 아래와 같이 바꾸는 게 더 좋을 것 같다는 생각입니다...!

 [title, images].every((arr) => arr.length > 0);

Copy link
Contributor

Choose a reason for hiding this comment

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

array를 여러 개 받는다는게 무슨 뜻인지는 모르겠지만(?) 위 함수의 사용 예제는 아래와 같습니다.

lengthValidate('arg1', 'arg2', ''); // false

내부적으로는 for...of 보다는 언급해주신 every가 더 나을 것 같습니다.

const lengthValidate = (...args: string[]) => {
  return args.every((arg) => arg.length > 0);
}

또한, 특정 타입(string)의 유효성 확인에 관한 내용이므로 이름을 좀 더 명확하게 한다면 validateStringLength도 가능할 것 같네요.

const validateStringLength = (length: number, ...args: string[]) => {
  return args.every((arg) => arg.length > length);
}

validateStringLength(3, 'test', 'test2'); // true
validateStringLength(5, 'test', 'test2'); // false

Copy link
Contributor Author

@d0422 d0422 Aug 31, 2024

Choose a reason for hiding this comment

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

저는 lengthValidate의 역할을 이터러블 객체의 길이 유효성을 검사하는 것으로 이해하였는데요,
그러나 실제 함수의 역할은 다수의 이터러블 객체를 받아 해당 객체의 length를 검사하는 것으로 보입니다.

그렇다면 단일한 객체의 유효성 검사가 아니기에 제안해주신 함수가 공통 util이 되려면 아래와 같은 부분이 조금 더 반영되어야 한다고 생각합니다. 아니라면 해당 함수를 사용하기 애매한 상황이 올 수 있고, 함수 변경으로 이어질 수 있을 것 같아요.

  1. iterableListLengthValidate로 이름을 변경
  2. lengthValidate 보다는 isEmptyArray와 같은 더 명확한 역할을 가진 함수로 변경하여 validate 함수에서 해당 함수를 호출하여 검사

Copy link
Contributor

Choose a reason for hiding this comment

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

아래와 같은 예외가 있을 수 있다는 것을 제가 놓쳤네요.

validateStringLength(3, [1, 2, 3, 4]); // true

@d0422 d0422 merged commit 7ee0d12 into main Sep 1, 2024
1 check passed
@d0422 d0422 deleted the feat/map-record-post branch September 1, 2024 04:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat 새로운 기능 추가
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants