Feat/#300 best piickle #301

ilmerry · 2023-07-06T14:51:46Z

브랜치명, 브랜치 알맞게 설정
Reviewer, Assignees, Label, Milestone, Issue(PR 작성 후에) 붙이기
PR이 승인된 경우 해당 브랜치는 삭제하기

📌 내용

'베스트피클더보기 페이지' 뷰를 완성합니다

페이지 내에서 크게는 랭킹 부분과 추천 부분으로 나뉘는데요, 각각 BestPiickleRank와 BestPiickleRecommend로 명명하였습니다
Rank의 각 요소들을 RankItem 컴포넌트로 세분화했어요 (RecommendItem도 리팩토링 예정)

공통컴포넌트 일부를 수정합니다

HeadingTitleContainer 수정: paddingVerticalValue와 routePath를 옵셔널 프로퍼티로 추가합니다.
- 꽤 많은 부분에서 쓰이는 공통컴포넌트이면서 동시에 padding 상하 부분이 padding이 조금씩 다르더라구요. 기본값은 2.4rem이고, 이와 다를 경우에는 paddingVerticalValue를 rem 값 기준으로 전달해주시면 됩니다.
- 메인페이지에서 '베스트피클'과 '무드피클' 페이지를 각각 라우팅해주어야 합니다. 기존에 to={routePaths.Cateogry}로 하드코딩 되어있던 부분을 routePath로 수정했어요.
BestPiickleCard 수정: new디자인을 반영했어요.

기타

개발과정에서 디자인관련해 문의사항이 생겨 슬랙에 질문해두었어요. 디자인시스템에도 문제가 있다는걸 발견했는데, 이 부분은 나연님과 지은님이 논의한 후 결정될 것 같습니다.

📌 내가 알게 된 부분

스타일만 바꿨을 시에는 커밋메시지에 [ design ] 으로 명시해야하는데 [ fix ] 로 적어버렸네요🥹 다음번부터는 꼭 지키겠습니다 !
컴포넌트 상으론 BestPiicklePage라고 명시했지만, 기획상 정확한 명칭은 '베스트피클더보기 페이지' 였네요.

📌 질문할 부분

/best 페이지에서 헤더부분의 메뉴를 누르면 에러가 나는 경우가 있었습니다. (지금은 또 잘 되네요..😅)
컴포넌트마다 ./style import 방식이 달라 고민하다가 일단 import * as St from './style' 로 개발하였습니다. 해당 방식과 `import St from './style' (./style 파일에서 St객체를 export 하는 방식)중 어떤 방식으로 할지 정하면 제가 통일해놓을게요
이건 중요한 질문인데, 개발하면서 다른 부분 참고하면서 간혹 가다가 position: fixed ...로 클릭부분을 지정해놓은 부분을 발견했어요. QA 과정에서 클릭범위를 지정하기 위해 일부러 그렇게 하신 것인지 궁금합니다!

📸 스크린샷

NYeonK · 2023-07-06T15:08:40Z

우와아아앙😆 개발 부분 공유를 엄청 자세히 해주셨네요!! 짱짱✨✨
중간리뷰는 내일 낮에 진행하겠습니다!

(+) 질문할 부분 1번 저도 같은 오류가 발생해서, social_login_2 PR에 적어 놓았어요!
2번은 import * as St from './style'가 가장 최근에 진행하고 있는 import 방식이 맞아요~ 파일도 많고, 어차피 UI가 계속 바뀌어서, 개발을 하다가 자신이 맡은 부분 import가 예전 방식이면 변경하고 있었어요!

NYeonK

은형이 평일에 일정 많은데!!!! 열심히 하고 있구나 믓찌다 증말 ♡〜٩( ˃́▿˂̀ )۶〜♡
api 연결두 파이티이이이이잉~!~!!!!

NYeonK · 2023-07-07T01:45:35Z

src/@components/MainPage/BestPiickle/BestPiickleCard/style.ts

-  color: ${({ theme }) => theme.colors.white};
-  background-color: ${({ theme }) => theme.newColors.gray900};
-  backdrop-filter: blur(0.6rem);
+  color: ${({ theme }) => theme.colors.gray700};


theme.newColors로 쓰면 좋을 것 같아요!

수정했습니다 감사합니다!

NYeonK · 2023-07-07T01:52:57Z

src/util/best/bestPiickleTitles.ts

+export const rankTitles: HeadingTitle = {
+  title: "베스트 피클 랭킹",
+  content: "가장 많이 북마크한 대화주제를 확인해보세요",
+  isMoreBtn: false,
+};
+
+export const recommendTitles: HeadingTitle = {
+  title: "이런 베스트 피클은 어때요?",
+  content: "",
+  isMoreBtn: false,
+};


분리 아주 좋아요~:D
베스트피클 페이지는 더보기 버튼이 없어서, isMoreBtn은 삭제해도 좋을 것 같네요!

이거 따로 빼낸 이유가 뭘까요??
상수 선언이 해당하는 컴포넌트에서만 사용되는데, 그 곳에서 사용하는 게 읽기 편하지 않을까요??/

저도 이전에 "정리"를 위한 상수 분리를 많이 했었는데,
그렇게 되면 문제가 해당 파일에서 확인하면 되는 것을 한 번 뎁스를 가지고 타고 와야 한다는 것이 시간적으로 문제가 된다는 것이에요

다시 말해서, (지금은 단순 const 객체이지만, 이후 분리, 모듈화를 진행할 때)
재사용, next engineer를 위한 모듈화가 되어야지, 단순 정리를 위한 서랍장을 만드는 것은, 그 컨텍스트를 이해하기 위해 서랍장을 만드는 사람의 의도를 파악까지 해야하는 문제가 생겨요

MainPage 만 참고하고 headingTitles 상수를 따로 선언했는데, 지금 다른 컴포넌트들을 보니 그냥 상단에서 선언했네요!
정리를 위한 상수 분리는 확실히 피로도만 올리는 작업이겠네요. 앞으로 유념해야겠습니다 !

나연님이 말씀하신대로 isMoreBtn은 불필요하긴 한데, 없으면 타입 에러가 나더라고요. 그래서 type에서 isMoreBtn만 옵셔널 프로퍼티로 바꾸고, 아래에서 주함님이 말씀하신대로 routePath여부에 따라 undefined 검사를 하도록 하겠습니다.

의견 감사해요!

코드와 타협하는 방향은 좋지 않다고 생각돼요
그 isMoreBtn 타입이 있았던 이유가 라우트 패쓰 옵션이 없었기 때문에 더보기가 있나없나를 판별하기 위해 있던 인터페이스였고,
라우트 패쓰가 있다 = 더보기 버튼이 있다 라는 도메인이 성립이 된다면, (저는 된다 생각돼요)
isMoreBtn 인터페이스 자체를 없애주는 방향으로 가는게 맞지 않나 싶어요??

NYeonK · 2023-07-07T01:55:43Z

src/@components/BestPiicklePage/index.tsx

+      <SuspenseBoundary>
+        <BestPiickleRank />
+        <BestPiickleRecommend />
+      </SuspenseBoundary>


얼.. SuspenseBoundary 바로 적용해서 쓰네! 멋있다은형이!!!!!

다른 페이지들에서 잘 적용되어있어서 많이 참고했어! ㅎㅎㅎ

NYeonK · 2023-07-07T02:00:58Z

src/@components/BestPiicklePage/BestPiickleRank/style.ts

+  ${({ theme }) => theme.newFonts.body4};
+  color: white;
+  text-align: center;
+  letter-spacing: -0.056rem; /* todo: 흠.. 폰트왜 다르지 */


ㅎㅎ.. 채민이한테 이야기 해야긋다! body4랑 letter-spacing만 다른 건가요?-?

전체적으로 letter-spacing이 다른 것 같아요!

NYeonK · 2023-07-07T02:04:43Z

src/@components/BestPiicklePage/BestPiickleRank/RankItem/style.ts

+  display: flex;
+  justify-content: center;
+  align-items: center;
+  width: 2.8rem;
+  height: 2.8rem;
+  cursor: pointer;


굿굿~! style 코드짤 때, 관련된 것끼리 띄어쓰기 해주면 좋을 것 같아요!

export const BookmarkWrapper = styled.div` width: 2.8rem; height: 2.8rem; display: flex; justify-content: center; align-items: center; cursor: pointer; `;

넵! 감사합니다😄

NYeonK · 2023-07-07T02:05:24Z

src/@components/BestPiicklePage/BestPiickleRank/RankItem/style.ts

+  ${({ theme }) => theme.newFonts.body4};
+  color: ${({ theme }) => theme.newColors.gray900};
+
+  // 말줄임표설정


이 주석도 임시로 적어둔걸까요?-? 아니라면 없애주세용~!

넵 수정했습니다!

NYeonK · 2023-07-07T06:11:41Z

src/core/routes/path.ts

@@ -17,4 +18,4 @@ export const routePaths = {
  Join_UserInfo: "user-info",
 };

-export type RoutePaths = typeof routePaths[keyof typeof routePaths];
+export type RoutePaths = (typeof routePaths)[keyof typeof routePaths];


옹 이거 괄호치면 뭔가 다른가요?-?

Prettier 자동완성이라 없으면 오류가 나는데, 저만 그런걸까요?!

NYeonK · 2023-07-07T06:48:28Z

src/@components/BestPiicklePage/BestPiickleRank/RankItem/index.tsx

@@ -26,7 +34,7 @@ export default function RankItem(props: RankItemProps) {
      <St.BookmarkWrapper onClick={toggleBookmark}>
        <IcBookmarkCheck_16_20 isChecked={isBookmarked} />
      </St.BookmarkWrapper>
-      <St.RankItemLink type="button" />
+      <St.RankItemLink type="button" onClick={() => navigateCardCollection(idx)} />


이 부분이 이어서 베스트 피클 카도 보기 버튼이죠? navigateCardCollection 대신 다른 함수명은 어떨까요?-?

navigateRankCollection 으로 변경하겠습니다! 좋은 의견이에요:)

NYeonK · 2023-07-07T06:51:37Z

src/@components/@common/HeadingTitleContainer/index.tsx


  return (
-    <St.Container>
+    <St.Container paddingVerticalValue={paddingVerticalValue ?? 2.4}>


세심한 사람..‧⁺( ᵒ̴̶̷̥́ ◡ ᵒ̴̶̷̣̥̀ )⁺‧

NYeonK · 2023-07-07T07:02:57Z

src/@components/BestPiicklePage/BestPiickleRecommend/index.tsx

+
+export default function BestPiickleRecommend() {
+  const { bestPiickle } = useBestPiickle();
+  const { scrollableContainerProps, isDragging } = useScrollableContainer(); // 요거 먼가요??


베스트피클을 보면 좌우 슬라이드가 가능한데요! 그 부분을 라이브러리 쓰지 않고 overflow-x: scroll;로 만들었어요! 그래서 데스크탑에서도 드래그 가능하게끔 구현한 hook입네다!
다시 한 번 주함오빠 최고 ( ﾟヮﾟ)/ ( ﾟヮﾟ)/

맞습니다! 정확히는 마우스 끌기로 스와이핑하기 위한 이벤트 핸들러를 정의해주는 훅이에요
이제 보니 useDraggingContainer ? 처럼 이름이 바뀌면 좋을 것 같네요!
바꿔주셔도 좋습니다

역시 이주함,,, useDraggingContainer로 변경하겠습니다!

이건 개인적인 의견인데, 이런 라이브러리 대체용? 훅들은 따로 lib폴더에서 따로 관리하는 경우도 있더라구요.
그리고 거기에 있는 기능들은 실제 라이브러리를 구현할 때처럼 개발해, 경험삼아 연습하기에도 좋다고 합니다!

오오 좋은데요??!
저도 모듈화나 배포에 대한 견문이 부족해서!!
여유가 된다면 이후에 한번 해봐주시고 공유해주셔도 굉장히 좋을 것 같습니다🥹🥹

joohaem

역시역시👍🏼👍🏼👍🏼👍🏼 고생 많았어요

혹시 PR 문서를 쓰는 데에 시간이 많이 걸리지는 않나요??
상세히 설명해주는 것은 좋지만, 정말 알아야 할 부분들만 컴팩트하게 적어야 쓰는 사람도 읽는 사람도 효율적으로 공유할 수 있지 않을까 싶어요! (부담없이 써도 된다는 이야기)

/best 페이지에서 헤더부분의 메뉴를 누르면 에러가 나는 경우가 있었습니다. (지금은 또 잘 되네요..😅) 제가 라우팅에서 뭔가

말이 끊긴듯한..🔉🔉

이건 중요한 질문인데, 개발하면서 다른 부분 참고하면서 간혹 가다가 position: fixed ...로 클릭부분을 지정해놓은 부분을 발견했어요. QA 과정에서 클릭범위를 지정하기 위해 일부러 그렇게 하신 것인지 궁금합니다!

이게 어느 부분이죠??!

joohaem · 2023-07-08T01:13:20Z

src/util/best/bestPiickleTitles.ts

+export const rankTitles: HeadingTitle = {
+  title: "베스트 피클 랭킹",
+  content: "가장 많이 북마크한 대화주제를 확인해보세요",
+  isMoreBtn: false,
+};
+
+export const recommendTitles: HeadingTitle = {
+  title: "이런 베스트 피클은 어때요?",
+  content: "",
+  isMoreBtn: false,
+};


이거 따로 빼낸 이유가 뭘까요??
상수 선언이 해당하는 컴포넌트에서만 사용되는데, 그 곳에서 사용하는 게 읽기 편하지 않을까요??/

저도 이전에 "정리"를 위한 상수 분리를 많이 했었는데,
그렇게 되면 문제가 해당 파일에서 확인하면 되는 것을 한 번 뎁스를 가지고 타고 와야 한다는 것이 시간적으로 문제가 된다는 것이에요

다시 말해서, (지금은 단순 const 객체이지만, 이후 분리, 모듈화를 진행할 때)
재사용, next engineer를 위한 모듈화가 되어야지, 단순 정리를 위한 서랍장을 만드는 것은, 그 컨텍스트를 이해하기 위해 서랍장을 만드는 사람의 의도를 파악까지 해야하는 문제가 생겨요

joohaem · 2023-07-08T01:15:03Z

src/@components/BestPiicklePage/BestPiickleRank/RankItem/index.tsx

@@ -0,0 +1,40 @@
+import React, { useState } from "react";


import React 자동완성 때문인지 선언이 좀 있네요! 없어도 될 것 같아요

확인했습니다 감사합니다!

joohaem · 2023-07-08T01:17:37Z

src/@components/BestPiicklePage/BestPiickleRank/RankItem/index.tsx

+interface RankItemProps {
+  cardId: string;
+  content: string;
+  idx: number;
+}


idx가 아니라 rank가 되어야 RankItem에 맞는 인터페이스일 것 같아요

좋은 의견이에요! 감사합니다:)

joohaem · 2023-07-08T01:21:05Z

src/@components/BestPiicklePage/BestPiickleRecommend/index.tsx

+
+export default function BestPiickleRecommend() {
+  const { bestPiickle } = useBestPiickle();
+  const { scrollableContainerProps, isDragging } = useScrollableContainer(); // 요거 먼가요??


맞습니다! 정확히는 마우스 끌기로 스와이핑하기 위한 이벤트 핸들러를 정의해주는 훅이에요
이제 보니 useDraggingContainer ? 처럼 이름이 바뀌면 좋을 것 같네요!
바꿔주셔도 좋습니다

joohaem · 2023-07-08T01:48:48Z

src/@components/@common/HeadingTitleContainer/index.tsx

      <St.Wrapper ismore={headingTitles.isMoreBtn}>
        <St.Title>{headingTitles.title}</St.Title>
        <St.Content>{headingTitles.content}</St.Content>
      </St.Wrapper>
-      {headingTitles && headingTitles.isMoreBtn && (
-        <St.Link to={routePaths.Category} className={GTM_CLASS_NAME.mainMoodPiickleMore}>
+      {headingTitles && headingTitles.isMoreBtn && routePath && (


routePath 인터페이스를 뚫어서 유연하게 사용이 가능해졌ㅆ네!!!!
그러면- isMoreBtn 타입을 아예 없어도 될 것 같아요. routePath의 유무로 moreBtn이 있는지 확인하면 어떨까 합니다.

아하 옵셔널로 줄 필요도 없겠네요. 아예 삭제하겠습니다!!

그리고 위 comment에서 마지막 질문은 나연언니가 갠톡해줘서, 디자인팀과 논의 후 다시 결정하기로 했어요!
PR 문서는 .. 첫 PR이라서 정성스럽게 써봤어요 🤭ㅋㅋㅋㅋㅋ

joohaem · 2023-07-08T04:28:56Z

FYI)
너무 잘해줘서- 이야기하자면
'이전 코드는 이렇게 되어있는데.. 별론데..' 싶은 부분들 모두 슬랙 스레드나 이슈에 올려주셔서 논의해주셔도 좋아요! 혹은 맘대로 고치고 pr로 보여주셔도 좋아요!!!!

joohaem

(말 많아서 죄송합니다) 그~~ 새 작업을 하고, 새 리뷰를 요청하실 때는 다시 review request를 걸어주세요!
리뷰를 원하는지 아닌지 파악할 수 있게!!

joohaem · 2023-07-09T01:40:22Z

src/@components/@common/BestPiickleCard/index.tsx

+  const onClickCard = () => {
+    if (!canNavigate) return;
+    navigateCardCollection(idx);
+  };


(생각해보면 좋을 것 같아요)

따로 빼준 이유가 있나요??

handle~이 아니라, on~ 인 이유가 있나요??

두줄 이상이라면 따로 빼는게 가독성이 좋을 거라고 생각했습니다!

앗 습관상 on으로 적어버렸네요 ! 다른 핸들러들은 handle인것 확인했습니다. 바꿀게요!

joohaem · 2023-07-09T01:40:35Z

src/@components/@common/BestPiickleCard/index.tsx

+  isLast?: boolean;
+}
+
+export default function BestPiickleCard(props: BestPiickleCardProps) {


⛑
파일 분리 + 피처 작업은 커밋을 분리해줘야 변경 사항을 확인하기 수월할 것 같아요 !!

생각없이 해버렸네요 🥲 주의하겠습니다

joohaem · 2023-07-09T01:42:27Z

src/@components/@common/BestPiickleCard/index.tsx

+  };
+  idx: number;
+  canNavigate: boolean;
+  isLast?: boolean;


(생각해보면 좋을 것 같아요)
last card에 대한 내용을 컴포넌트로 분리해서 LastBestPiickleCard로 하지 않고,
컴포넌트 내에서 last card에 대한 내용을 하는 것이 옳은 인터페이스일까요?
장단점이 뭘까요???

해놓고 리팩토링한다는 걸 깜빡했네요😅
단점은 아무래도 파일이 많아지면서 번들 크기가 증가한다는 것일거고, 경우에 따라 props drilling문제도 생길 것 같아요!
그런데 지금 LastCard는 필요로하는 props가 없으니까 말씀하신대로 컴포넌트를 분리하는 게 좋을 것 같아요:)

joohaem · 2023-07-09T01:43:41Z

src/@components/@common/BestPiickleCard/index.tsx

+
+  return (
+    <St.Container type="button" className={GTM_CLASS_NAME[GTM_IDX_KEY]} onClick={onClickCard}>
+      {isLast ? (


변수명이 요게 맞나요??? 바뀐 것 같은..!

isLast 말씀하시는거 맞나요? 바뀌었다는게 무슨 말이죠?!

joohaem · 2023-07-09T01:44:37Z

src/@components/MainPage/BestPiickle/index.tsx

+                  bestPiickle={bestPiickle}
+                  idx={idx}
+                  canNavigate={!isDragging}
+                  isLast={idx !== 5}


slice(0, 5) 면 idx가 0-4 아닌가요.>~!?

로직을 잘못 생각했었어요! isLast이면 기본카드가 나오게 해놨었네요 !isLast로 고쳤어요
알주셔서 감삼다 (_ _)

Feat/#301 best piickle API

ilmerry added 10 commits July 4, 2023 19:57

[ feat ] BestPiicklePage 라우팅

2faa5b7

[ feat ] 베스트 피클 랭킹 컴포넌트 구현

f198e16

[ fix ] Boomark Wrapper 크기지정

d3ec52d

[ feat ] BestPiickleRecommend 구현

d9025a0

[ feat ] BestPiickleCard 디자인 수정 반영

a01fedb

[ feat ] 랭킹 navigation 추가

0738f0b

[ feat ] HeaddingTitleContainer 프로퍼티 추가

fd99cb4

[ fix ] routePath undefined 오류

ecb3ec1

[ fix ] 메인뷰 HeadingTitles 디자인

2d12afa

[ fix ] 베스트피클페이지 HeadingTitles padding값 적용

b283cfe

ilmerry added Feature 은형 labels Jul 6, 2023

ilmerry requested review from joohaem and NYeonK July 6, 2023 14:51

ilmerry self-assigned this Jul 6, 2023

ilmerry marked this pull request as draft July 6, 2023 14:52

NYeonK linked an issue Jul 6, 2023 that may be closed by this pull request

[ Best Piickle ] 베스트피클 페이지 구현 #300

Closed

4 tasks

NYeonK reviewed Jul 7, 2023

View reviewed changes

joohaem reviewed Jul 8, 2023

View reviewed changes

ilmerry added 2 commits July 8, 2023 13:11

[ refactor ] 중간 리뷰 반영

5b41210

[ refactor ] RecommendItem 분리

4431323

[ feat ] 베스트피클 마지막 카드

5d57e82

ilmerry marked this pull request as ready for review July 8, 2023 11:48

joohaem reviewed Jul 9, 2023

View reviewed changes

ilmerry mentioned this pull request Jul 10, 2023

Feat/#301 best piickle API #304

Merged

3 tasks

[ design ] 디자인 수정 반영

3766a60

ilmerry added a commit that referenced this pull request Jul 13, 2023

Merge branch 'feat/#300-best_piickle' into feat/#301-best_piickle

edc8fb4

ilmerry merged commit a48bb25 into release/2.1.0 Jul 16, 2023

ilmerry deleted the feat/#300-best_piickle branch July 16, 2023 12:40

ilmerry added a commit that referenced this pull request Jul 16, 2023

Merge pull request #304 from TeamPiickle/feat/#301-best_piickle

7248757

Feat/#301 best piickle API

Feat/#300 best piickle #301

Feat/#300 best piickle #301

Conversation

ilmerry commented Jul 6, 2023 • edited Loading

📌 내용

'베스트피클더보기 페이지' 뷰를 완성합니다

공통컴포넌트 일부를 수정합니다

기타

📌 내가 알게 된 부분

📌 질문할 부분

📸 스크린샷

NYeonK commented Jul 6, 2023 • edited Loading

NYeonK 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

ilmerry Jul 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilmerry Jul 8, 2023 • edited Loading

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

NYeonK Jul 7, 2023 • edited Loading

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

joohaem 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

joohaem commented Jul 8, 2023

joohaem 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

joohaem Jul 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilmerry commented Jul 6, 2023 •

edited

Loading

NYeonK commented Jul 6, 2023 •

edited

Loading

ilmerry Jul 8, 2023 •

edited

Loading

ilmerry Jul 8, 2023 •

edited

Loading

NYeonK Jul 7, 2023 •

edited

Loading

joohaem Jul 9, 2023 •

edited

Loading