[REFACTOR] 닉네임 설정 화면 UX 개선 및 리팩토링 #254

rbgksqkr · 2024-08-27T12:16:55Z

Issue Number

#252

As-Is

닉네임 글자수 제한 없었음
확인 버튼으로만 이동 가능
정상 링크인데 에러 UI가 보여졌다가 사라짐
방 생성하는데 joinable API 요청 발생

To-Be

닉네임 글자수 12자로 제한
닉네임 Enter로도 방 생성 및 참여 가능
잘못된 uuid로 판단하는 로직 수정
불필요한 joinable 요청 제거

Check List

테스트가 전부 통과되었나요?
모든 commit이 push 되었나요?
merge할 branch를 확인했나요?
Assignee를 지정했나요?
Label을 지정했나요?

Test Screenshot

(Optional) Additional Description

버튼 위치 변경은 디자인적인 문제라 올릴지 말지 의견 코멘트 바람~!

🌸 Storybook 배포 주소

https://woowacourse-teams.github.io/2024-ddangkong/storybook/

i

useon

방학 때도 개발하는 진정한 개발왕. 몇 가지 코멘트 달았고요. 일단 어프루브 드립니다요! 짱 😊👍

useon · 2024-09-02T08:19:00Z

frontend/src/pages/NicknamePage/NicknameInput/NicknameInput.styled.ts

+  width: 100%;
+  height: 4.8rem;
+  padding: 0 1rem;
+  border-radius: 1rem;


💭 질문 💭

Theme로 설정해둔 border-radius 값이 아닌 1rem으로 설정한 이유가 궁금해요 ! radius10(= 0.8rem)로 작성했을 때 의도한 ui가 아니었나요 ? 😄🤚

border-radius 테마 적용해놓은 걸 매번 까먹네요ㅜ 적용하겠습니다!

useon · 2024-09-02T08:22:04Z

frontend/src/pages/NicknamePage/NicknameInput/hooks/useNicknameInput.ts

+
+  const handleKeyDown = (event: React.KeyboardEvent<HTMLInputElement>) => {
+    if (event.key === 'Enter') {
+      handleMakeOrEnterRoom();


🌸 칭찬 🌸

Enter를 누르면 바로 방에 입장할 수 있도록 했으면 좋겠다는 사용자 피드백을 반영하셨군요 마루짱이다 ~ 🍀

useon · 2024-09-02T08:23:34Z

frontend/src/pages/NicknamePage/NicknameInput/hooks/useNicknameInput.ts

+
+  const handleChangeInput = (e: ChangeEvent<HTMLInputElement>) => {
+    if (e.target.value.length > NICKNAME_MAX_LENGTH) {
+      e.target.value = e.target.value.slice(0, NICKNAME_MAX_LENGTH);


💭 질문 💭

maxLength로 input 입력 값을 제어했는데 nickname max length보다 입력 값의 길이가 더 커지는 경우가 있었나요 ?? 🤓

한글은 maxLength를 걸어놔도 1글자가 넘어가는 현상이 발생해요! 명확하게 설명은 어렵지만 한글과 영어가 차지하는 byte 차이 때문인 것 같아요. 한글 1글자를 인코딩 방식에 따라 2byte 또는 3byte로 해석하는데, 초성의 경우 1byte로 해석하여 maxLength만큼 작성 후에도 초성이 작성되는 버그로 이해했어요!

아하 ! 그런 버그가 있다니 이해가 됩니다 🙌 친절 마루 칭찬합니다 🤭👍

useon · 2024-09-02T08:36:03Z

frontend/src/types/room.ts

@@ -20,6 +20,7 @@ export interface RoomInfo {
  isGameStart: boolean;
  roomSetting: RoomSetting;
  members: Member[];
+  master: Omit<Member, 'isMaster'>;


🌸 칭찬 🌸

오호 Omit 야무지게 타입 작성했네예 푸하하 ^ .^

useon · 2024-09-02T08:45:32Z

frontend/src/pages/NicknamePage/useMakeOrEnterRoom.ts

  const nicknameInputRef = useRef<HTMLInputElement>(null);
  const navigate = useNavigate();
  const [{ isMaster }, setMemberInfo] = useRecoilState(memberInfoState);
-  const [, setIsLoading] = useState(false);

  const [, setRoomUuidState] = useRecoilState(roomUuidState);


🙏 제안 🙏

해당 파일에서 setRoomUuidState만 사용하는 것 같은데 useSetRecoilState(roomUuidState) 로 수정하는 것은 어떨까요 ?

맞습니다!!! 그런데 제가 구현한 부분이 아니기도 하고, 해당 PR에서 의도한 건 버그 수정과 UX 반영 느낌이라서 코드 리팩토링 관련된 내용은 최대한 그대로 두긴 했습니다! 하나씩 고치다보면 전체를 다 고쳐야되는 상황이였어요 ㅎㅎ 지금보니 어떤건 고치고 어떤건 안고쳐서 리뷰어 입장에선 헷갈릴 것 같긴 하네용

닉네임 페이지에 대한 전체적인 리팩토링은 담당자와 함께 다른 이슈에서 작성하는 건 어떨까요??

마루 말대로 어떤 부분은 useSetRecoilState로 고쳐서 이 부분은 이유가 있었는지 궁금했어요! 마루 말대로 해당 PR은 버그 수정과 UX 반영이니 다른 이슈로 해결하면 더 좋을 것 같네요 🤭✨

useon · 2024-09-02T09:00:48Z

frontend/src/pages/NicknamePage/useMakeOrEnterRoom.ts

@@ -44,16 +40,15 @@ export const useMakeOrEnterRoom = (showModal: () => void) => {
    onSuccess: (data) => {
      setMemberInfo((prev) => ({ ...prev, memberId: data.member.memberId }));
      setRoomUuidState(data.roomUuid || '');
-      navigate(ROUTES.ready(Number(data.roomId)));
-      setIsLoading(false);
+      navigate(ROUTES.ready(Number(data.roomId)), { replace: true });


🙏 제안 🙏

해당 파일의 두 mutation에서 onSuccess 되었을 때 같은 동작을 하는 것으로 보여요. 같은 로직을 함수로 분리해서 관리해도 좋을 것 같네요 ! 두 mutation이 onSuccess와 onError 시 같은 로직을 수행하는데 API를 다르게 요청해서 어쩔 수 없는 중복이네요.. 흠 !! 더 나은 개선 방법에 대해서도 함께 고민해 봅시다 !!!! 🤚🤚😊

저도 동의합니다! 그런데 개인적인 생각으로는 해당 훅 자체가 분리되어야한다고 생각했습니다!
방 생성과 방 참여가 같이 있으니까 각각에 대한 처리가 어려워보입니다. 이 부분도 리팩토링을 진행하면 시간이 걸릴 것 같아 닉네임 화면 코드 리팩토링은 다음 이슈로 넘기는 게 어떨까요??

맞아요 .. 가장 좋은 것은 관심사에 따라 훅을 분리하는 것이라고 생각해요 ! 다음 이슈에서 해당 부분에 대한 리팩토링이 진행되면 좋겠네요 👍🙌

useon · 2024-09-02T09:06:23Z

frontend/src/pages/NicknamePage/NicknameInput/NicknameInput.tsx

+import { nicknameInput, nicknameInputContainer, nicknameLengthText } from './NicknameInput.styled';
+import createRandomNickname from '../createRandomNickname';
+
+const NICKNAME_MAX_LENGTH = 12;


💭 질문 💭

useNicknameInput 훅에서도 같은 상수를 선언해서 사용하는 것으로 보이는데 마루는 해당 부분에 대해 파일에 선언해서 작성하는 것을 더 선호하시나요 ?? 상수 관리에 대해 이야기 나눠보면 좋을 듯 합니돠 !!!!!!!! 🤭🤭

한번만 사용하면 파일 상단에 작성하는 편인 것 같아요! 그런데 2번이 사용되니 애매했는데, 또 마땅히 상수를 넣을만한 파일이 없어서 그냥 2개 만들었던 것 같아요,,,,,
아니면 time.ts 라는 파일 대신 config 와 같은 이름으로 정책에 대한 상수를 넣는 것도 좋아보여요! 어떻게 생각하시나용

오 좋아요 ! 마루 말대로 time 파일의 상수까지 포함하여 정책에 대한 상수 파일을 한번에 관리하는 것도 좋겠네요 🙌👍

novice0840

코드 확인하고 피드백 남깁니다!!
버그 수정하느라 고생 많았습니다~~

novice0840 · 2024-09-03T12:32:43Z

frontend/.storybook/preview.tsx

+        <RecoilRoot>
+          <ThemeProvider theme={Theme}>
+            <MemoryRouter initialEntries={['/']}>
+              <Global styles={GlobalStyle} />
+              <Story />
+            </MemoryRouter>
+          </ThemeProvider>
+        </RecoilRoot>


💭 질문 💭

storybook에서 recoil 상태를 사용해야 하는 경우가 생긴건가요?

storybook에서 recoil 상태를 사용한다기보단 storybook에서 반환하는 컴포넌트에서 recoil을 사용하기 때문에 없으면 오류가 발생합니다!

기존의 컴포넌트들도 해당 decorators로 감싸지기 때문에 storybook 환경에서도 오류가 안난걸로 이해하시면 될 것 같아요!

novice0840 · 2024-09-03T12:42:11Z

frontend/src/pages/NicknamePage/NicknameInput/hooks/useNicknameInput.ts

+    setNickname(e.target.value);
+  };
+
+  const handleKeyDown = (event: React.KeyboardEvent<HTMLInputElement>) => {


🙏 제안 🙏

위에서 Event를 선언한 방식과 동일하게
React.KeyboardEvent를 KeyboardEvent 로 바꾸는 건 어떤가요?

KeyboardEvent로 작성하면 window에서 제공하는 KeyboardEvent가 적용되어 제네릭 타입이 없다고 오류가 뜨더라구요

래퍼런스가 명확하게 없는데 한번더 테스트해보고, 안되면 테스트 결과도 같이 첨부하겠습니다!

ChangeEvent는 React에서만 존재해서 원래 React.ChangeEvent인데 namespace에서 그대로 가져오고, KeyboardEvent는 window에도 존재해서 window.KeyboardEvent를 우선적으로 참조하는 것 같아요! input에 KeyboardEvent로 설정하면 다음과 같은 오류가 발생합니다.

제네릭 타입을 지정하지 않고 KeyboardEvent만 작성하면 해당 코드에선 오류가 발생하지 않는데, input의 onKeyDown과 타입이 맞지 않는 오류가 발생합니다.

novice0840 · 2024-09-03T12:59:10Z

frontend/src/pages/NicknamePage/NicknameInput/hooks/useNicknameInput.ts

+  const handleChangeInput = (e: ChangeEvent<HTMLInputElement>) => {
+    if (e.target.value.length > NICKNAME_MAX_LENGTH) {
+      e.target.value = e.target.value.slice(0, NICKNAME_MAX_LENGTH);


🙏 제안 🙏

혹시 12 글자를 넘으면 slice를 하는 것 보다 12글자 이하일 때만 setNickname를 하는 것은 어떤가요?
이런 식이면 slice를 안 써도 될 것 같습니다.

if (e.target.value.length <= NICKNAME_MAX_LENGTH) { setNickname(e.target.value); }

오호 이 방법도 괜찮네용 한번 한글로 실험해보고 적용해보도록 할게요!

훨씬 이해하기 쉬운 것 같아 로직 수정하였습니다~!

novice0840 · 2024-09-03T13:01:58Z

frontend/src/pages/NicknamePage/useMakeOrEnterRoom.ts

    },
    onError: () => {
      showModal();
    },
  });

  const handleMakeOrEnterRoom = () => {
-    const nickname = nicknameInputRef.current?.value || randomNickname;
+    const nickname = nicknameInputRef.current?.value || nicknameInputRef.current?.placeholder || '';


🌸 칭찬 🌸

이렇게 하면 원래 2번 사용하던 randomNickname 값을 한 번만 사용할 수 있겠군요!

novice0840 · 2024-09-03T13:14:03Z

frontend/src/pages/NicknamePage/NicknamePage.tsx

@@ -44,7 +43,7 @@ const NicknamePage = () => {
    }
  }, [roomUuid, setRoomUuidState]);

-  if (roomUuid && !data?.isJoinable)
+  if (!isJoinableLoading && roomUuid && !data?.isJoinable)


🌸 칭찬 🌸

불필요한 에러 UI 제거 굿굿

rbgksqkr added 13 commits August 27, 2024 21:02

refactor: 불필요한 setState 제거 #252

9bff3be

fix: roomInfo 목데이터 응답값 필드명 수정 #252

e1609e8

feat: 닉네임 길이를 12자로 제한 #252

05a435e

feat: NicknameInput 컴포넌트 분리 및 한글 maxLength 넘는 문제 처리 #252

7104bb9

design: nicknameInput 컴포넌트 반영 및 레이아웃 수정 #252

c36baf0

feat: Enter 눌러도 방 참가 API가 호출되는 기능 구현 #252

cb66add

i

refactor: handleKeyDown을 nicknameInput 역할로 분리 #252

eb20884

refactor: 에러 메세지 수정 #252

b9b6d9a

refactor: roomUuid가 있을 때만 참여 가능 API 호출 #252

8f5cd1e

fix: nickname 입력하지 않았을 때 값을 못가져오는 문제 해결 #252

48e1b52

refactor: 방 참여 가능 여부를 체크할 때 로딩상태일 때도 에러 UI 보여주는 문제 해결 #252

7056bbe

refactor: 방 설정 정보 응답 타입에 master 필드 추가 #252

404bc65

fix: recoil 사용하는 컴포넌트에서 발생하는 스토리북 빌드 오류 해결 #252

c91112f

rbgksqkr added ♻️ refactor 리팩토링 🫧 FE front end labels Aug 27, 2024

rbgksqkr requested review from useon and novice0840 August 27, 2024 12:16

rbgksqkr self-assigned this Aug 27, 2024

rbgksqkr linked an issue Aug 27, 2024 that may be closed by this pull request

[REFACTOR] 닉네임 설정 화면 UX 개선 및 리팩토링 #252

Closed

4 tasks

useon approved these changes Sep 2, 2024

View reviewed changes

novice0840 approved these changes Sep 3, 2024

View reviewed changes

rbgksqkr added 3 commits September 10, 2024 01:13

refactor: border radius theme으로 적용 #252

0a16448

refactor: 정책과 관련된 상수 config 파일로 분리 #252

d0d1523

refactor: 한글 입력 오류 로직 수정 #252

52f43d5

rbgksqkr merged commit abd80b7 into develop Sep 9, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REFACTOR] 닉네임 설정 화면 UX 개선 및 리팩토링 #254

[REFACTOR] 닉네임 설정 화면 UX 개선 및 리팩토링 #254

rbgksqkr commented Aug 27, 2024 •

edited

Loading

useon left a comment

useon Sep 2, 2024

rbgksqkr Sep 2, 2024

useon Sep 2, 2024

useon Sep 2, 2024

rbgksqkr Sep 2, 2024

useon Sep 6, 2024

useon Sep 2, 2024

useon Sep 2, 2024

rbgksqkr Sep 2, 2024

useon Sep 6, 2024

useon Sep 2, 2024

rbgksqkr Sep 2, 2024

useon Sep 6, 2024

useon Sep 2, 2024

rbgksqkr Sep 2, 2024

useon Sep 6, 2024

novice0840 left a comment

novice0840 Sep 3, 2024

rbgksqkr Sep 3, 2024

novice0840 Sep 3, 2024

rbgksqkr Sep 3, 2024

rbgksqkr Sep 9, 2024

novice0840 Sep 3, 2024

rbgksqkr Sep 3, 2024

rbgksqkr Sep 9, 2024

novice0840 Sep 3, 2024

novice0840 Sep 3, 2024

[REFACTOR] 닉네임 설정 화면 UX 개선 및 리팩토링 #254

[REFACTOR] 닉네임 설정 화면 UX 개선 및 리팩토링 #254

Conversation

rbgksqkr commented Aug 27, 2024 • edited Loading

Issue Number

As-Is

To-Be

Check List

Test Screenshot

(Optional) Additional Description

🌸 Storybook 배포 주소

useon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

💭 질문 💭

Choose a reason for hiding this comment

Choose a reason for hiding this comment

🌸 칭찬 🌸

Choose a reason for hiding this comment

💭 질문 💭

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

🌸 칭찬 🌸

Choose a reason for hiding this comment

🙏 제안 🙏

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

🙏 제안 🙏

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

💭 질문 💭

Choose a reason for hiding this comment

Choose a reason for hiding this comment

novice0840 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

💭 질문 💭

Choose a reason for hiding this comment

Choose a reason for hiding this comment

🙏 제안 🙏

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

🙏 제안 🙏

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

🌸 칭찬 🌸

Choose a reason for hiding this comment

🌸 칭찬 🌸

rbgksqkr commented Aug 27, 2024 •

edited

Loading