[Refactor] 아카이빙 페이지 리팩토링 #208

namdaeun · 2024-08-03T15:05:20Z

해당 이슈 번호

closed #189

체크리스트

🔀 PR 제목의 형식을 잘 작성했나요? e.g. [feat] PR을 등록한다.
💯 테스트는 잘 통과했나요?
🏗️ 빌드는 성공했나요?
🧹 불필요한 코드는 제거했나요?
✅ 컨벤션을 지켰나요?
💭 이슈는 등록했나요?
🏷️ 라벨은 등록했나요?
💻 git rebase를 사용했나요?
🙇‍♂️ 리뷰어를 지정했나요?

📌 내가 알게 된 부분

컴포넌트들을 모두 모아둔 페이지 컴포넌트(ArchivingPage.tsx)안에 각 컴포넌트에 대한 코드가 너무 드러난 것 같아서 가독성에 좋지 못하다고 판단했습니다. 그래서 최대한 이를 분리하는 것에 신경써봤어요. (각 컴포넌트 코드 안에 그 컴포넌트에 해당하는 로직 코드가 존재하도록 !)
이게 정답일지는 모르겠지만 제 눈에는 좀 더 깔꼼해보이긴 합니다. 여러분들의 의견이 궁금하네욤

📌 질문할 부분

📌스크린샷 (선택)

wuzoo

리팩토링 고생하셨습니다 ! LGTM ~

wuzoo · 2024-08-03T17:11:58Z

src/page/archiving/index/ArchivingPage.style.ts

+    overflowX: 'hidden',
+    overflowY: 'hidden',


overflow: hidden ?

wuzoo · 2024-08-03T17:13:28Z

src/page/archiving/index/ArchivingPage.tsx

-            currentMonth={selectedMonth}
-            onMonthClick={(month) => setSelectedMonth(month)}
+            currentMonth={selectedMonthType}
+            onMonthClick={(month) => setSelectedMonthType(month)}


onMonthClick={setSelectedMonthType}

을 의도하신 걸까요 ?

움 넵 그렇습니다 수정할게요!

wuzoo · 2024-08-04T07:59:48Z

src/page/archiving/index/ArchivingPage.tsx


  const [selectedBlock, setSelectedBlock] = useState<Block>();
  const timeBlocks: Block[] = data?.timeBlocks || [];
-  const blockFloors = alignBlocks(timeBlocks, endDay, selectedMonth, currentYear);
+  const blockFloors = alignBlocks(timeBlocks, endDay, selectedMonthType, currentYear);


alignBlocks에 대한 결과값을 useMemo로 처리해주는 것도 좋을 것 같아요. 실제로 비교적 높은 비용의 연산이기도 하니 메모이제이션을 적용해준다면 성능에 좋을 것 같습니다.

또한 Daysection과 같은 컴포넌트들은 실제로 prop이 바뀔 일이 흔하지 않은데, 부모 요소의 리렌더링으로서 같이 모두다 리렌더링되니까, memo과 같은 함수를 통해 메모이제이션 해줄 수도 있을 것 같아요

추가적으로 selectedMonthType이 "12월" 과 같은 문자열을 리턴하는 것으로 보여지는데, MonthType보다 더 적절한 네이밍은 없을까용 ?

보통 Date가 아닌 문자열 형식으로 포맷된 Date 문자열을 dateString 이라고 하니까 MonthString...?

useMemo로 블록 쌓기 로직 처리해주는 것과 DaySection도 memo를 통해 렌더링 최소화 하는 것 모두 동의합니다 반영했어요 !

그리고 selectedMonthType의 경우 저도 네이밍을 많이 고민해봤는데 .. selectedMonthString도 나쁘지 않은데 문자열이라는 타입에 초점을 두기 보다는 달 헤더의 각 이름을 의미하는 바로 selectedMonthLabel은 어떠신가요 ?

wuzoo · 2024-08-04T08:11:05Z

src/page/archiving/index/component/TimeBlock/TimeBlock.tsx

    <div css={blockStyle(blockWidth, startPosition, floor, color, isSelected)} onClick={onBlockClick} {...props}>
      {BLOCK_TYPE.find((icon) => icon.name === blockType)?.icon}
-      <p css={spanStyle}>{children}</p>
+      <span css={blockNameStyle}>{children}</span>
    </div>
  );
 };


제가 생각하기에는 타임블록은 clickable하고 focusable 해야된다고 생각해요. 왜냐하면 그래야 스크린리더도 해당 타임블록들이 클릭 가능하며, 유저와의 인터랙션을 통해 부가적인 기능들을 제공한다는 것을 인식할 수 있을 것 같아요.

따라서 적절한 role을 부여하여 keyboard 이벤트와 click이벤트 모두다 적용해주면 어떨까요 ? tab을 통해서도 접근 가능하다면 사용자 입장에서나 접근성 측면에서나 더 좋을 것 같다고 생각해요

넵 접근성 고려하는 거 너무 좋은 생각입니다 !! 추가해서 적용해뒀습니다 감사합니다 :)

rtttr1

깔끔하게 리펙토링 잘하셨네요~~ 최적화까지 신경쓰신게 아주 대단합니다 저도 메모이제이션을 고려해보면서 코드를 한번 다시 슥 봐야겠어요!

rtttr1 · 2024-08-04T13:59:16Z

src/page/archiving/index/ArchivingPage.tsx

@@ -35,7 +41,10 @@ const ArchivingPage = () => {
    setSelectedId(id);
  };

-  const handleBlockClick = (e: React.MouseEvent<HTMLDivElement>, block: Block) => {
+  const handleBlockClick = (
+    e: React.MouseEvent<HTMLDivElement> | React.KeyboardEvent<HTMLDivElement>,


keyboardEvent 까지 신경쓴거 아주 굳이네요 굳

rtttr1 · 2024-08-04T14:01:20Z

src/page/archiving/index/ArchivingPage.tsx

-        paddingLeft: '6rem',
-      }}
-      css={{ overflowY: 'hidden', overflowX: 'hidden' }}>
+    <Flex css={pageStyle}>


깔꼼해졌네요

css와 styles를 둘 다 썼길래 css하나로 통일해줬습니다리

rtttr1 · 2024-08-04T14:19:50Z

src/page/archiving/index/ArchivingPage.tsx

+  const blockFloors = useMemo(
+    () => alignBlocks(timeBlocks, endDay, selectedMonthString, currentYear),
+    [currentYear, endDay, selectedMonthString, timeBlocks]
+  );


오 usememo로 최적화까지! 역시 리액트 스장님이십니다

역시 스장님!!

Bowoon1216

아카이빙 페이지 전체 리팩토링하느라 수고하셨습니다 ❤️
보고싶네용 😉

Bowoon1216 · 2024-08-04T22:27:07Z

src/page/archiving/index/component/DaySection/DaySection.tsx

+        return (
+          <Flex css={dayStyle(isEven, isToday)}>
+            <Flex css={dayHeaderStyle(isToday)}>{day + 1}</Flex>
+            <Flex css={bodyStyle} />


소소하게 궁금한게, 여기 Flex 태그는 내용이 없는데도 css가 적용이 되나용 ❓

그러게용 순수 css prop 만을 사용할거라면 가시적일수 있도록 일반 html tag를 사용해도 좋을 것 같네요.

Flex 태그는 디폴트로 div 태그의 속성들을 상속하기 때문에, 일반적인 prop들 다 적용 가능해요 !

헉 보운이 눈썰미 대박 필요 없는 코드였어요 수정하겠습니다 !!🔥

그러게용 순수 css prop 만을 사용할거라면 가시적일수 있도록 일반 html tag를 사용해도 좋을 것 같네요.

저는 flex 속성을 사용할 때 가능하면 Flex 공컴을 써보려고 했는데 고런 부분도 함 생각해봐야겠네요 고맙습니다 ~

cindy-chaewon

고생하셨습니다!! 최고!!

cindy-chaewon · 2024-08-06T13:44:32Z

src/page/archiving/index/ArchivingPage.tsx

    useDate();

-  const selectedMonthNumber = parseInt(selectedMonth.split('월')[0]);
+  const selectedMonth = parseInt(selectedMonthString.split('월')[0]);


오오 string으로 구분해주니, string 과 number을 바로 구분하여 사용할 수 있겠네요!!

cindy-chaewon · 2024-08-06T13:45:36Z

src/page/archiving/index/ArchivingPage.tsx

+  const blockFloors = useMemo(
+    () => alignBlocks(timeBlocks, endDay, selectedMonthString, currentYear),
+    [currentYear, endDay, selectedMonthString, timeBlocks]
+  );


역시 스장님!!

cindy-chaewon · 2024-08-06T13:58:40Z

src/page/archiving/index/component/TimeBlock/TimeBlock.style.ts

@@ -28,7 +28,7 @@ export const blockStyle = (width: number, startPosition: number, floor: number,
    cursor: 'pointer',
  });

-export const spanStyle = {
+export const blockNameStyle = {
  margin: 'auto 0.7rem',

  lineHeight: '120%',


span태그가 기본적으로 inline 요소이고 줄바꿈이 되지 않으니, lineHeight: '120%',는 없애도 되지 않을까요..??!

동의합니다 수정했어요 :)

…g-page

namdaeun added 10 commits July 31, 2024 21:17

#187 refactor: DaySection 관련 코드 분리

1eb619a

#187 style: 안 쓰이는 css 코드 제거

1a171b3

#187 refactor: 배열의 index를 day + 1로 변경

ab7b7e4

#187 refactor: 변수명 변경

fb86287

#187 docs: 충돌 해결

afbc945

#187 refactor: css 분리

9d8b7a4

feat: 첫번째 줄 블록과 타임라인 사이 간격 띄움

d119c7f

#187 chore: 줄바꿈 추가

df15822

#189 docs: merge develop

8cf2c89

#189 chore: 빌드에러 해결

4eef360

namdaeun added ♻️Refactor 💫다은 labels Aug 3, 2024

namdaeun requested review from rtttr1, cindy-chaewon, Bowoon1216 and wuzoo August 3, 2024 15:05

namdaeun self-assigned this Aug 3, 2024

wuzoo reviewed Aug 4, 2024

View reviewed changes

namdaeun added 4 commits August 4, 2024 18:09

#187 feat: DaySection memo로 메모이제이션

0c22187

#205 refactor: 상태 변수명 변경

050f880

#205 feat: 타임블록 tab과 enter로 접근 가능하도록 구현

db64404

#205 chore: 불필요한 코드 제거

e12cdc5

rtttr1 approved these changes Aug 4, 2024

View reviewed changes

Bowoon1216 approved these changes Aug 4, 2024

View reviewed changes

cindy-chaewon approved these changes Aug 6, 2024

View reviewed changes

namdaeun and others added 2 commits August 7, 2024 18:05

#187 chore: 필요없는 스타일 코드 제거

4ac3dcd

Merge branch 'develop' into refactor/archiving/#187-refactor-archivin…

818af30

…g-page

namdaeun merged commit bc92416 into develop Aug 7, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Refactor] 아카이빙 페이지 리팩토링 #208

[Refactor] 아카이빙 페이지 리팩토링 #208

namdaeun commented Aug 3, 2024 •

edited

Loading

wuzoo left a comment

wuzoo Aug 3, 2024

namdaeun Aug 4, 2024

wuzoo Aug 3, 2024

namdaeun Aug 4, 2024

wuzoo Aug 4, 2024

wuzoo Aug 4, 2024

namdaeun Aug 4, 2024 •

edited

Loading

wuzoo Aug 4, 2024

namdaeun Aug 4, 2024

rtttr1 left a comment

rtttr1 Aug 4, 2024

rtttr1 Aug 4, 2024

namdaeun Aug 5, 2024

rtttr1 Aug 4, 2024

cindy-chaewon Aug 6, 2024

Bowoon1216 left a comment

Bowoon1216 Aug 4, 2024

wuzoo Aug 5, 2024

namdaeun Aug 5, 2024

namdaeun Aug 5, 2024 •

edited

Loading

cindy-chaewon left a comment

cindy-chaewon Aug 6, 2024

cindy-chaewon Aug 6, 2024

cindy-chaewon Aug 6, 2024

namdaeun Aug 7, 2024 •

edited

Loading

[Refactor] 아카이빙 페이지 리팩토링 #208

[Refactor] 아카이빙 페이지 리팩토링 #208

Conversation

namdaeun commented Aug 3, 2024 • edited Loading

해당 이슈 번호

체크리스트

📌 내가 알게 된 부분

📌 질문할 부분

📌스크린샷 (선택)

wuzoo 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

namdaeun Aug 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtttr1 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

Bowoon1216 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

namdaeun Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

cindy-chaewon 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

namdaeun Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

namdaeun commented Aug 3, 2024 •

edited

Loading

namdaeun Aug 4, 2024 •

edited

Loading

namdaeun Aug 5, 2024 •

edited

Loading

namdaeun Aug 7, 2024 •

edited

Loading