[월간조회] 데스크탑뷰 QA이슈 해결 #20

JeongWon-CHO · 2024-10-13T12:16:53Z

Close [월간조회] 데스크탑뷰 QA이슈 해결 #19

What is this PR? 🔍

기능 : 날짜 이동 버튼 추가 / 엑셀 다운로드 모달창 수정 / 모바일 뷰 구조 수정
issue : [월간조회] 데스크탑뷰 QA이슈 해결 #19

Changes 📝

'날짜 이동 버튼'추가

‘오늘' 버튼을 누르면 오늘 날짜가 있는 주 or 달 로 이동합니다.
‘<’, ‘>’ 버튼을 누르면 다른 주 or 달의 식단을 모두 조회할 수 있습니다.

엑셀 다운로드 모달창 수정

모달창 설명 문구 변경

'식단은 2022/11/29 부터 다운받을 수 있어요.' 로 수정되었습니다.

엑셀 다운로드 에러 툴팁 추가

시작일 또는 종료일이 오늘 날짜 이후를 나타내는 경우
시작일이 2022/11/29 이전의 날짜인 경우
시작일이 종료일 이후인 경우

주간/월간 식단 오타 수정

월간일 때는 '월간 식단', 주간일 때는 '주간 식단'이 뜨도록 수정하였습니다.

모바일뷰 구조 수정

오늘 버튼을 추가함으로써 주간/월간 선택 버튼이 상단으로 이동하였습니다.
상단 설정 버튼을 메뉴 아이콘으로 바꾸고 상세 페이지로 이동하는 것이 아닌, 드롭다운으로 변경하였습니다.

ScreenShot 📷

기본 상태입니다.

호버 상태입니다.

변경된 모달창 설명 문구입니다.

시작일 또는 종료일이 오늘 날짜 이후를 나타내는 경우입니다.

시작일이 2022/11/29 이전의 날짜인 경우입니다.

시작일이 종료일 이후인 경우입니다.

월간일 때는 '월간 식단', 주간일 때는 '주간 식단'이 뜹니다.

주간/월간 선택 버튼이 상단으로 이동하였습니다.

상단 설정 버튼을 메뉴 아이콘으로 바꾸고, 드롭다운으로 변경하였습니다.

Test CheckList ✅

Excel 다운 에러 1 : 시작일 또는 종료일이 오늘 날짜 이후를 나타내는 경우
Excel 다운 에러 2 : 시작일이 2022/11/29 이전의 날짜인 경우
Excel 다운 에러 3 : 시작일이 종료일 이후인 경우

✔️ Please check if the PR fulfills these requirements

It's submitted to the correct branch, not the develop branch unconditionally?
If on a hotfix branch, ensure it targets main?
There are no warning message when you run yarn lint

daepan

기능 추가를 위해 React-Tooltip을 예쁘게 잘 사용하셨군요. 수고하셨습니다.
코드 내용에 대해서 몇가지 피드백을 남겨봤습니다. 잘 고민하시고 수정해주시면 감사하겠습니다.

src/layout/Header/Dropdown.tsx

src/layout/Header/index.tsx

src/pages/Coop/components/Calendar/index.tsx

src/pages/Coop/components/DownloadModal/index.tsx

haejinyun

수고하셨습니다!!

이것저것 보다보니 여러 리뷰를 달았네요.. 지금 당장 수정해야할 것도 고민해보면 좋은 것도 달아보았습니다! 확인하시고 괜찮다고 생각신 것, 적용할 것만 적용해주세요!

제가 해당 프로젝트 테스트 아이디가 없어서 접속을 해서 확인을 해보지 못했습니다..
리뷰 확인시 해당 부분 고려해주시면 감사하겠습니다..! 죄송합니다ㅠㅠ

src/layout/Header/Dropdown.module.scss

src/layout/Header/index.tsx

haejinyun · 2024-10-14T01:46:10Z

src/layout/Header/index.tsx

      <nav className={styles.header__content}>
-        <Setting1 />
+        {isMobile ? (


얘는 제가 사이트 계정을 몰라서 들어가서 보지는 못했네요ㅠㅠ
만약 모바일에서만 보이는 것이라면 &&를 써서 표현하는 것도 좋을 것 같아요.
아니면 모바일과 웹이 아주 많이 다르다면 아예 컴포넌트를 따로 파서 조건부 렌더링을 해주는 것도 나쁘지 않은 방법이라고 생각합니다~ ( 이건 추후 상세한 개발 시에 고려해주세요~)

모바일과 데스크탑의 헤더가 다르기 때문에 이렇게 작성하였습니다..!

src/layout/Header/index.tsx

src/pages/Coop/components/Calendar/Calendar.module.scss

haejinyun · 2024-10-14T01:59:12Z

src/pages/Coop/components/Calendar/index.tsx

-            <div key={day} className={styles.day}>{day}</div>
-          ))}
+    <div>
+      {isMobile ? (


코드가 상당한데 혹시 모바일과 웹의 디자인이 많이 다른가요..?
이것도 제가 아이디가 없어서 확인은 못해봤지만ㅜㅠㅜ
많이 다르다면 컴포넌트를 따로 파주는 것이 편할 것 같기도 하네요...

많이 다르지 않다면, 두 부분에서 다른 쪽만 조건부로 감싸는건 어떤가요?
삼항연산자로 다르게 렌더링 시키는 즉 영역을 줄여보는 것이지요~

여러 부분이 작게 작게 달라서 컴포넌트로 따로 빼기도 애매하고 조건부도 애매하다고 생각하여서 이렇게 했습니다...!!

src/pages/Coop/components/DateMover/index.tsx

src/pages/Coop/components/DiningDownload/index.tsx

src/pages/Coop/components/DownloadModal/DownloadModal.module.scss

haejinyun · 2024-10-14T02:04:42Z

src/pages/Coop/components/DownloadModal/index.tsx

@@ -60,7 +66,7 @@ export default function DownloadModal({ closeModal }: DownloadModalProps) {
    if (!date.year || !date.month || !date.day) {
      return '';
    }
-    return `${date.year}-${date.month.padStart(2, '0')}-${date.day.padStart(2, '0')}`;
+    return `${date.year.toString()}-${date.month.toString().padStart(2, '0')}-${date.day.toString().padStart(2, '0')}`;


이렇게 각각 toString으로 변하게 할꺼면 위에서 타입을 지정할 때 string으로 하지 않은 이유는 무엇인가요?

날짜 입력 칸에 문자가 들어가는 것을 방지하기 위해 이렇게 했습니다..!
그런데 api는 날짜를 string 으로 받아서 변환 과정이 필요했습니다..

MinGu-Jeong · 2024-10-15T05:56:25Z

src/layout/Header/index.tsx

+                {/* 월간조회 기능 추가 시 활성화 */}
+                {/* <SearchIcon className={styles.search} /> */}


이건 다음 기획에 예정되어있는 기능인가요?

네 맞습니다!

MinGu-Jeong · 2024-10-15T05:57:39Z

src/layout/Header/index.tsx

+          </div>
+        ) : (
+          <div>
+            <Setting1 />


Setting1말고 더 적절한 네이밍은 없을까요?
코드만 보고 어떤 컴포넌트가 보여질지 잘 예상이 안가는것 같습니다

HeaderNavigation으로 변경하고 header 컴포넌트 하위로 해당 컴포넌트 위치 변경했습니다!

D0Dam

첫 이슈였는데 그래도 잘 해결해 주셨습니다! 수고하셨습니다!

junghaesung79

고생 많으셨어요!!!
나중에 어프루브는 트랙장님께 부탁드리면 좋을 것 같아요~~

junghaesung79 · 2024-10-15T13:21:24Z

src/pages/Coop/components/Calendar/Calendar.module.scss

+.form-toggle-button-mobile {
+  display: flex;


클래스명으로써 네 어절은 조금 긴 것 같아요. 저는 보통 1~2 어절로 사용하는 것을 좋아하는데 어떻게 생각하시나요?

junghaesung79 · 2024-10-15T13:35:44Z

src/pages/Coop/components/Calendar/Calendar.module.scss

+  font-size: 14px;
+  font-style: normal;
+  font-weight: 400;
+  line-height: 160%;


font에는 여러 속성이 있죠. 그 속성들은 기본값을 가지고 있어요. 이 기본 값은 명시적으로 적을 수도, 생략할 수도 있어요. 저는 예전에는 적어주다가 지금은 생략하는 것을 선호해요. 그 이유는 아무래도 기본 값이기 때문에 일반적인 상황에서 기본 값을 갖고 있는 경우가 많기 때문이예요. 지금 보시면 normal과 400이 기본 값인데, 한번 이 값이 기본 값임을 의식한 뒤로는 지우게 되더라구요. 하지만 이 부분에 대해서 팀에서 약속된 내용은 없어서 그냥 한 번 나는 어떤 방법이 더 좋은가 생각해보시면 좋을 것 같아요😁
제가 말한 기본 값으로 어떤 값을 갖는 지 한 번 찾아보세요~

참고

junghaesung79 · 2024-10-15T13:42:16Z

src/pages/Coop/components/Calendar/index.tsx


+  const prevMonth = () => {
+    const prevMonthDate = new Date(selectedDate);


함수 이름을 바꿔주면 더 좋을 것 같아요! 함수는 보통 동작하는 영어를 자주 사용하는데 지금의 명명은 그런 느낌이 들지 않는 것 같아요. 그리고 이 함수의 이름을 위해 이 코드 위쪽에 반환 값의 이름을 변경하여 사용하셨는데, 이럴 경우 나중에 리팩토링을 진행할 때 실수가 발생할 수도 있을 것 같아요. 맥락은 잘 모르지만 go 이런 것을 앞에 붙일 수 있을 것 같아요!

junghaesung79 · 2024-10-15T13:45:27Z

src/pages/Coop/components/Calendar/index.tsx

+    prevCalendarMonth();
+    setSelectedDate(prevMonthDate);
+  };


오잉? 이것도 함수명이었군요.. 더욱 실수하면 안 되는 코드네요.

junghaesung79 · 2024-10-15T13:51:24Z

src/pages/Coop/components/Calendar/index.tsx

+                <button
+                  type="button"
+                  key={date.toString()}
+                  className={cn({
+                    [styles.date]: true,


살짝 걱정이 드는 게, 달력을 보여주다보면 이전 달 말일이나 다음 달 초의 날짜가 이번 달 날짜와 겹쳐서 키가 중복되지는 않나요? 만약 키가 겹치면 개발자도구에서 경고를 보여줄 거예요.

junghaesung79 · 2024-10-15T13:55:57Z

src/pages/Coop/components/DateMover/DateMover.module.scss

+.button-prev,
+.button-next {


차라리 둘 다 같은 class명을 붙여서 이렇게 콤마 없이 스타일 을 줄 수 있을 것 같아요!

junghaesung79 · 2024-10-15T13:56:20Z

src/pages/Coop/components/DateMover/DateMover.module.scss

+  font-weight: 500;
+  line-height: 160%; /* 28.8px */


주석~
한 번 깃허브 커밋 해보시겠어요? 이런 식으로 suggestion을 할 수 있는데 이때 여기서 바로 커밋이 가능하거든요!

Suggested change

font-weight: 500;

line-height: 160%; /* 28.8px */

font-weight: 500;

line-height: 160%;

왼쪽의 Commit suggestion을 한 번 눌러보세요!

junghaesung79 · 2024-10-15T13:59:52Z

src/pages/Coop/components/DateMover/DateMover.module.scss

+
+  &:hover {
+    background-color: #e1e1e1;
+    border-radius: 999px;


음... 상관 없긴 한데 저는 둥근 모양의 요소에도 정확한 radius를 먹여요ㅋㅎㅋㅎ 근거는 굳이 말하자면 읽을 때 크기 예상 가능?

junghaesung79 · 2024-10-15T14:01:33Z

src/pages/Coop/components/DateMover/index.tsx

+      <button
+        className={styles['button-today']}
+        type="button"
+        onClick={onTodayClick}


정해진 건 아니지만, on~ 속성에 함수를 넣을 때 저는 handle~ 로 이름 붙일 것 같아요

daepan

수고하셨습니다.

JeongWon-CHO added 4 commits October 11, 2024 23:36

fix: 월간조회 데스크탑뷰 QA 이슈 해결

75e516f

feat: 날짜 이동 버튼 추가

fef942d

feat: 엑셀 다운로드 버튼에 툴팁 추가

a4aacbf

fix: lint 에러 수정

e0c5550

github-actions bot requested review from daepan and haejinyun October 13, 2024 12:17

JeongWon-CHO changed the title ~~Feat/#19 desktop view qa issue~~ [월간조회] 데스크탑뷰 QA이슈 해결 Oct 13, 2024

fix: lint 에러 수정

27e845e

daepan requested changes Oct 14, 2024

View reviewed changes

haejinyun reviewed Oct 14, 2024

View reviewed changes

JeongWon-CHO and others added 4 commits October 14, 2024 21:03

fix: 모바일 드롭다운 컴포넌트 이름 변경

d30f1e3

refactor: QA 반영

7b09aa9

refactor: 엑셀 다운로드 기능 부분 수정

8160ad9

fix: lint 에러 수정

c66eef7

MinGu-Jeong reviewed Oct 15, 2024

View reviewed changes

refactor: Setting1 네이밍 변경 및 위치 이동

0ab3e87

D0Dam requested a review from MinGu-Jeong October 15, 2024 06:19

D0Dam assigned D0Dam and JeongWon-CHO Oct 15, 2024

D0Dam added ✨ Feature 기능 개발 🔨 Refactor 코드 리팩토링 labels Oct 15, 2024

JeongWon-CHO added 5 commits October 15, 2024 20:58

fix: 스타일 수정

803938f

fix: 코드 중복 제거

4c17f44

fix: z-index 값 수정 및 css 주석 제거

11f874b

fix: 커스텀 훅 사용

7cc0159

refactor: 조건부 렌더링을 props 전달 방식으로 변경

78f9e6a

JeongWon-CHO requested a review from daepan October 15, 2024 13:56

fix: lint 에러 수정

8c0e25f

D0Dam approved these changes Oct 15, 2024

View reviewed changes

junghaesung79 reviewed Oct 15, 2024

View reviewed changes

daepan approved these changes Oct 15, 2024

View reviewed changes

Merge branch 'develop' into feat/#19-desktop-view-QA-issue

7827e00

JeongWon-CHO merged commit 2b15f7a into develop Oct 15, 2024
1 check passed

github-actions bot deleted the feat/#19-desktop-view-QA-issue branch October 15, 2024 14:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[월간조회] 데스크탑뷰 QA이슈 해결 #20

[월간조회] 데스크탑뷰 QA이슈 해결 #20

JeongWon-CHO commented Oct 13, 2024 •

edited

Loading

daepan left a comment

haejinyun left a comment

haejinyun Oct 14, 2024

JeongWon-CHO Oct 15, 2024

haejinyun Oct 14, 2024

JeongWon-CHO Oct 15, 2024

haejinyun Oct 14, 2024

JeongWon-CHO Oct 15, 2024

MinGu-Jeong Oct 15, 2024

D0Dam Oct 15, 2024

MinGu-Jeong Oct 15, 2024

D0Dam Oct 15, 2024

D0Dam left a comment

junghaesung79 left a comment

junghaesung79 Oct 15, 2024

junghaesung79 Oct 15, 2024

junghaesung79 Oct 15, 2024

junghaesung79 Oct 15, 2024

junghaesung79 Oct 15, 2024

junghaesung79 Oct 15, 2024

junghaesung79 Oct 15, 2024

junghaesung79 Oct 15, 2024

junghaesung79 Oct 15, 2024

junghaesung79 Oct 15, 2024

daepan left a comment

		{/* 월간조회 기능 추가 시 활성화 */}
		{/* <SearchIcon className={styles.search} /> */}


		const prevMonth = () => {
		const prevMonthDate = new Date(selectedDate);

		.button-prev,
		.button-next {

[월간조회] 데스크탑뷰 QA이슈 해결 #20

[월간조회] 데스크탑뷰 QA이슈 해결 #20

Conversation

JeongWon-CHO commented Oct 13, 2024 • edited Loading

What is this PR? 🔍

Changes 📝

'날짜 이동 버튼'추가

엑셀 다운로드 모달창 수정

주간/월간 식단 오타 수정

모바일뷰 구조 수정

ScreenShot 📷

Test CheckList ✅

✔️ Please check if the PR fulfills these requirements

daepan left a comment

Choose a reason for hiding this comment

haejinyun 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

D0Dam left a comment

Choose a reason for hiding this comment

junghaesung79 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

daepan left a comment

Choose a reason for hiding this comment

JeongWon-CHO commented Oct 13, 2024 •

edited

Loading