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

[#215] Introduce utils for creating review #216

Merged
merged 5 commits into from
Mar 8, 2024

Conversation

2wheeh
Copy link
Collaborator

@2wheeh 2wheeh commented Feb 29, 2024

Resolves #215

Changes

  • @lexical/headless 패키지를 추가합니다. 테스트 외에도 SSR을 위해 사용될 예정이라 devDependency 가 아닌 dependency 로 추가 합니다.
  • validateReviewFields 의 인터페이스는 zodsafeParse 를 참고했습니다. (참고: https://zod.dev/?id=safeparse)
  • editorState 의 경우 Lexical Editor 가 돌려준 EditorState 객체임에도 유효하지 않은 경우를 예상했습니다. 이 때 content: 'content has something wrong' 를 리턴합니다.
  • 다음과 같은 패턴으로 사용됩니다.
const validatedFields = validateReviewFields({ title, movieName, editorState });

if (!validatedFields.success) {
    setFieldState({ error: validatedFields.errors });
    // ...
    return;
}

setFieldState({ error: null });
void createReviewAndNavigate(validatedFields.data);
2024-03-01.5.00.16.mov

@2wheeh 2wheeh added the ui Something about UI label Feb 29, 2024
@2wheeh 2wheeh self-assigned this Feb 29, 2024
Copy link

@isutare412
Copy link
Collaborator

혹시 "title is empty" 경고가 인풋을 넣자마자 사라지게 할 수도 있나요?

@2wheeh
Copy link
Collaborator Author

2wheeh commented Mar 1, 2024

혹시 "title is empty" 경고가 인풋을 넣자마자 사라지게 할 수도 있나요?

예시의 구현은 validateReviewFieldssetFieldState 를 fetch 전에 한번만 호출하고 있는데요, 키 입력이 발생할때 마다 호출하도록 변경하면 되겠네요.

validateReviewFields 호출 후 결과에 따라 등록하기의 disable 상태를 결정하면 됩니다.
약간의 성능 저하와 더 자연스러운 ux 간의 tradeoff 가 되겠네요.

@isutare412
Copy link
Collaborator

예시의 구현은 validateReviewFields 와 setFieldState 를 fetch 전에 한번만 호출하고 있는데요, 키 입력이 발생할때 마다 호출하도록 변경하면 되겠네요.

validateReviewFields 호출 후 결과에 따라 등록하기의 disable 상태를 결정하면 됩니다.
약간의 성능 저하와 더 자연스러운 ux 간의 tradeoff 가 되겠네요.

전체 필드를 하나의 validation 으로 처리하지 않고 필드마다 별도로 처리하면 필요한 부분만 다시 validation 할 수 있지 않을까요?

@2wheeh
Copy link
Collaborator Author

2wheeh commented Mar 8, 2024

@isutare412

이 validation 함수를 양쪽(키 입력, 제출)에서 모두 사용하게 되면 너무 빈번하고 즉각적인 에러 alert으로 유저 경험에 좋지 않다고 판단했고, 여러 서비스의 UI를 참고해서 시나리오를 다시 생각해봤습니다.

  1. 키 입력 (onChange) 에는 maxLength 에 대한 에러를 처리
  2. empty field 에 대한 검사는 submit 시에 처리 -> 이 PR의 validation 함수 사용
  3. alert 의 경우 유저 입력과 무관하게 일정시간 노출 후 사라짐
submit-uii.mov

즉 validation 이 두 단계로 나누어 져야합니다.

  • onChange 로 각 필드의 maxLength 에 대한 확인은 개별 컴포넌트 내에서 처리하고
  • 현재 PR의 validation 함수는 submit 에 사용되는 data 한번에 처리 (empty case)
// on keyStroke

const onChange = (e: React.ChangeEvent<HTMLTextAreaElement>) => {
    // other logics ...

    // limit to 20 characters
    if (newText.length > MAX_MOVIE_NAME_LENGTH) {
      newText = newText.slice(0, MAX_MOVIE_NAME_LENGTH)

      ref.current?.focus();
      debouncedEmitToast(`${'영화 제목' }${MAX_MOVIE_NAME_LENGTH}자 이하로 입력해주세요.`, 'error');
    }

    setValue(newText);
  };
// onClick submit button

const validatedFields = validateReviewFields({ title, movieName, editorState });

if (!validatedFields.success) {
    // Order to check should be aligned with the order of elements in the DOM
    // title -> movieName -> editor
    if (validatedFields.errors.title) {
      titleRef.current?.focus();
      emitToast(validatedFields.errors.title, 'error');
    } else if (validatedFields.errors.movieName) {
      movieNameRef.current?.focus();
      emitToast(validatedFields.errors.movieName, 'error');
    } else {
      editorRef.current?.focus();
      emitToast(validatedFields.errors.content!, 'error');
    }

    return;
}

void createReview(validatedFields.data)

전체 필드를 하나의 validation 으로 처리하지 않고 필드마다 별도로 처리하면 필요한 부분만 다시 validation 할 수 있지 않을까요?

해당 validation 함수를 매번 키 입력 마다 호출하는 경우 피드백 주신 방향이 합리적 입니다. 그런데 이 함수가 제출 시에만 호출된다해도 여전히 쪼개야 할까요??

참고1. 네이버 블로그

naver-empty.mov
naver-exceed.mov

참고2. 카카오 브런치

brunch-empty.mov
brunch-exceed.mov

참고3. 미디엄

medium-empty.mov

@2wheeh 2wheeh force-pushed the feature/issue-215/utils-create-review branch from 4351872 to 6485f63 Compare March 8, 2024 06:55
Copy link

github-actions bot commented Mar 8, 2024

Copy link

github-actions bot commented Mar 8, 2024

@isutare412
Copy link
Collaborator

해당 validation 함수를 매번 키 입력 마다 호출하는 경우 피드백 주신 방향이 합리적 입니다. 그런데 이 함수가 제출 시에만 호출된다해도 여전히 쪼개야 할까요??

아하 제시해주신 여러 예시에서는 필드 validation 에러 메세지가 잠시만 노출되고 사라지는 것 같은데 맞을까요?
현재 올려주신 영상에서는 다른 플랫폼의 예시와 달리 에러 메세지가 사라지지 않아서 어색함을 느꼈던 것 같습니다.

Copy link

github-actions bot commented Mar 8, 2024

@2wheeh
Copy link
Collaborator Author

2wheeh commented Mar 8, 2024

아하 제시해주신 여러 예시에서는 필드 validation 에러 메세지가 잠시만 노출되고 사라지는 것 같은데 맞을까요?
현재 올려주신 https://github.com/MovieReviewComment/Mr.C/pull/216#issue-2162009231에서는 다른 플랫폼의 예시와 달리 에러 메세지가 사라지지 않아서 어색함을 느꼈던 것 같습니다.

@isutare412 네 처음 구상했던 UI (PR본문의 영상) 대로면 어색하게 느끼신 지점을 해소하기위해 onChange 마다 error field 를 초기화하는 구현이 필요하고, 이를 위해선 말씀해주신대로 validtion 함수를 쪼개는 방향이 합리적입니다.

추가적으로 제가 해당 validation 함수를 onChange 마다 호출하여 사용하는 방향으로 생각하고 있어서 더 혼란이 있었네요.

현재 정리된 제 생각은 위의 댓글과 같습니다.

@isutare412
Copy link
Collaborator

추가적으로 제가 해당 validation 함수를 onChange 마다 호출하여 사용하는 방향으로 생각하고 있어서 더 혼란이 있었네요.

현재 정리된 제 생각은 위의 댓글과 같습니다.

넵 vlaidation 함수를 쪼개고 onChange 마다 호출하는 것에 대해 찬성합니다.

@2wheeh
Copy link
Collaborator Author

2wheeh commented Mar 8, 2024

추가적으로 제가 해당 validation 함수를 onChange 마다 호출하여 사용하는 방향으로 생각하고 있어서 더 혼란이 있었네요.

현재 정리된 제 생각은 위의 댓글과 같습니다.

넵 vlaidation 함수를 쪼개고 onChange 마다 호출하는 것에 대해 찬성합니다.

@isutare412 음 제가 의견을 잘못 전달드린 것 같아요.
현재 validation 함수는 현재 한번에 처리하는 방식을 유지하면서 제출 시에만 호출하고,
onChange 에 사용할 로직은 이 pr의 validation 함수와는 별개로 각 필드에 대한 컴포넌트 내부에서 구현하는 방향을 말씀드린 거였습니다.

@isutare412
Copy link
Collaborator

현재 validation 함수는 현재 한번에 처리하는 방식을 유지하면서 제출 시에만 호출하고,
onChange 에 사용할 로직은 이 pr의 validation 함수와는 별개로 각 필드에 대한 컴포넌트 내부에서 구현하는 방향을 말씀드린 거였습니다.

아하 이해하였습니다.
컴포넌트에 들어가는 개별 validation 은 별도의 PR 에서 진행되는 것이군요.

#216 (comment)

alert 의 경우 유저 입력과 무관하게 일정시간 노출 후 사라짐

현재 PR 본문의 영상에서는 일정시간 노출 후 사라지지 않고 유지되고 있는데, 이는 아직 토스트 컴포넌트가 없기 때문인거죠?

@2wheeh
Copy link
Collaborator Author

2wheeh commented Mar 8, 2024

@isutare412 네 처음엔 PR본문의 방식을 생각했는데, 추가 조사 후 #216 (comment) 에 첨부한 영상의 방식으로 계획을 바꿨고, 직접 눈으로 확인해보기 위해서 구현해서 첨부했습니다.

Copy link
Collaborator

@isutare412 isutare412 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다 👍

@skgndi12
Copy link
Collaborator

skgndi12 commented Mar 8, 2024

@2wheeh 님 고생하셨습니다. 저는 디자인에 대한 짧은 의견이 있어 글을 남깁니다. 위 코멘트에서 @2wheeh 님께서 다른 서비스에서의 대한 에러 노출 방식을 영상으로 남겨주신 것을 확인했습니다. 지금의 에러 메시지가 노출되는 방식도 좋지만, 저 개인적으로는 브런치의 에러 노출 방식이 가장 좋다고 생각합니다. 길이에 대한 에러 같은 경우 페이지의 위에서 빨간색이 아닌 문구로 애니메이션으로 사용자의 주의를 끄는 것이 가장 좋은 디자인이지 않을까 생각이 드네요. 이건 어디까지나 제 의견이니까 @2wheeh 님의 판단대로 결정해주시면 감사하겠습니다.

@2wheeh
Copy link
Collaborator Author

2wheeh commented Mar 8, 2024

@2wheeh 님 고생하셨습니다. 저는 디자인에 대한 짧은 의견이 있어 글을 남깁니다. 위 코멘트에서 @2wheeh 님께서 다른 서비스에서의 대한 에러 노출 방식을 영상으로 남겨주신 것을 확인했습니다. 지금의 에러 메시지가 노출되는 방식도 좋지만, 저 개인적으로는 브런치의 에러 노출 방식이 가장 좋다고 생각합니다. 길이에 대한 에러 같은 경우 페이지의 위에서 빨간색이 아닌 문구로 애니메이션으로 사용자의 주의를 끄는 것이 가장 좋은 디자인이지 않을까 생각이 드네요. 이건 어디까지나 제 의견이니까 @2wheeh 님의 판단대로 결정해주시면 감사하겠습니다.

네 상단에 바 형태로 보여지는 UI를 말하신 것으로 이해하면 될까요? 고려해보겠습니다 의견 감사합니다.

@2wheeh 2wheeh force-pushed the feature/issue-215/utils-create-review branch from 3da8ec2 to 5c39694 Compare March 8, 2024 14:32
Copy link

github-actions bot commented Mar 8, 2024

Copy link

github-actions bot commented Mar 8, 2024

@2wheeh 2wheeh merged commit 542efd0 into develop Mar 8, 2024
4 checks passed
@2wheeh 2wheeh deleted the feature/issue-215/utils-create-review branch March 8, 2024 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ui Something about UI
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Introduce utils to build data for request for creating review
3 participants