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

Drag and Drop Component 만들기 #49

Merged
merged 15 commits into from
May 16, 2023
Merged

Drag and Drop Component 만들기 #49

merged 15 commits into from
May 16, 2023

Conversation

sumi-0011
Copy link
Member

@sumi-0011 sumi-0011 commented May 8, 2023

🤔 해결하려는 문제가 무엇인가요?

디자인이 나오기 전에 임시로 Drag and Drop Component 만들어 놓기

🎉 변경 사항

  • feature/createSurvey/cardList 폴더에 CardList, Card Component 생성
  • CardList 스토리북 추가
  • CardList Dnd 동작 테스트, 추가 버튼 동작 테스트 코드 작성

🙏 여기는 꼭 봐주세요!

cardList 테스트

드래그 앤 드롭 이후 정말로 데이터가 바뀌었는지 테스트하는 것이 필요한데, 어떻게 테스트하면 좋을지 모르겠습니다.. 😢
혹시 아시는분 계신가요

아직 명세서가 확정되지 않아서 추가하는 부분과, Type부분은 임시로 넘어갔습니다.

emotion을 사용하는 과정에서 에러가 발생합니다

사용된 다른 곳과 똑같이 사용했는데, 에러가 발생하는 이유를 모르겠어요 😢

'{ children: Element[]; css: SerializedStyles; }' 형식은 'DetailedHTMLProps<HTMLAttributes<HTMLDivElement>, HTMLDivElement>' 형식에 할당할  없습니다.
  'DetailedHTMLProps<HTMLAttributes<HTMLDivElement>, HTMLDivElement>' 형식에 'css' 속성이 없습니다.

스크린샷 2023-05-09 오전 1 06 24

🌄 스크린샷

dnd

참고

@github-actions
Copy link

github-actions bot commented May 8, 2023

Bundle Sizes

Compared against 0132b1c

Route: No significant changes found

Dynamic import: None found.

@sjoleee sjoleee added the feat New feature or request label May 8, 2023
@sjoleee
Copy link
Member

sjoleee commented May 8, 2023

draft지만 몇가지 남겨봅니다!

  • emotion 및 import구문 린트 오류 저는 안나는데 다른분들 환경에서도 한번 확인해주시면 좋을거같아요! import 린트 오류는 1번째 라인에서 파일명을 react로 인식해서 발생하는 것 같습니다?

  • 컴포넌트 구조를 조금 더 고민해보면 좋을 것 같아요!

image

시안에서 dnd가 가능한 carddnd불가능한 card가 같은 container에 담겨있는데, 지금 구조라면 아래처럼 사용하게 될 것 같아요.

<Container>
  <Card>
  <Card>
  <Card>
  <Dnd listItems={listItems} />
</Container>

Card 컴포넌트를 컴파운드패턴으로 만들고, wrapping해서 사용하는 방법은 어떨까유??
코드는 대충 느낌만...

// CardList

const CardList = (props) => {
return (
  <Reorder.Group values={items} onReorder={setItems} as="section">
    {items.map((item, idx) => (
      <Card dnd={idx > 3}>
        <Card.Title>{props.title}<Card.Title>
        <Card.Content>{props.content}<Card.Content>
        {idx > 3 && <ReorderIcon dragControls={dragControls} /> }
      </Card>
      )
    }
  </Reorder.Group>
  )
}
  • 테스트는 순서 옮길때마다 state가 잘 바뀌는지를 중점적으로 보면 좋지 않을까 싶습니다~~! fireEvent에 drag 관련된 이벤트들이 있는걸로 아는데 그걸 사용하면 되지 않을까 싶네유~~!

@sumi-0011
Copy link
Member Author

draft지만 몇가지 남겨봅니다!

  • emotion 및 import구문 린트 오류 저는 안나는데 다른분들 환경에서도 한번 확인해주시면 좋을거같아요! import 린트 오류는 1번째 라인에서 파일명을 react로 인식해서 발생하는 것 같습니다?
  • 컴포넌트 구조를 조금 더 고민해보면 좋을 것 같아요!
image

시안에서 dnd가 가능한 carddnd불가능한 card가 같은 container에 담겨있는데, 지금 구조라면 아래처럼 사용하게 될 것 같아요.

<Container>
  <Card>
  <Card>
  <Card>
  <Dnd listItems={listItems} />
</Container>

Card 컴포넌트를 컴파운드패턴으로 만들고, wrapping해서 사용하는 방법은 어떨까유?? 코드는 대충 느낌만...

// CardList

const CardList = (props) => {
return (
  <Reorder.Group values={items} onReorder={setItems} as="section">
    {items.map((item, idx) => (
      <Card dnd={idx > 3}>
        <Card.Title>{props.title}<Card.Title>
        <Card.Content>{props.content}<Card.Content>
        {idx > 3 && <ReorderIcon dragControls={dragControls} /> }
      </Card>
      )
    }
  </Reorder.Group>
  )
}
  • 테스트는 순서 옮길때마다 state가 잘 바뀌는지를 중점적으로 보면 좋지 않을까 싶습니다~~! fireEvent에 drag 관련된 이벤트들이 있는걸로 아는데 그걸 사용하면 되지 않을까 싶네유~~!

오 이거 보고 생각하다가, 제가 잘못 생각한 부분이 있다는걸 깨달았네요! 감사합니다 ㅎㅎ
그런데 이런 방식으로 사용하면 밑의 사진과 같이 카드가 4개이고 그중 위 3개는 순서 변경을 하지 않는 카드라서 Reorder 범위에서 없어야 할 것 같아요.

image

그렇게 되면

return (
    <>
      <Card />
      <Card />
      <Card />
      <Reorder.Group values={items} onReorder={setItems} as="section">
        {items.map((item) => (
          <Card />
        ))}
      </Reorder.Group>
    </>
  );

이런식으로 코드를 작성해야할 것 같은데,
여기서 '컴파운드 컴포넌트를 사용할지' vs 'ReorderIcon 컴포넌트를 option props로 넘길지' 두 방법이 생각이 나는데
어떤 방식이 좋을까요?!

@sjoleee
Copy link
Member

sjoleee commented May 9, 2023

엇 Reorder.Group 내부에서도 요소들을 Reorder.Item인가 그걸로 감싸놓지 않으면 DND 영향 안받는걸로 아는데 아닌가유??

Card에 ReorderIcon을 prop으로 만들면 코드 중복을 좀 더 줄일 수 있을 것 같고, 컴파운드로 만들면 확장에 용이한 형태가 될 것 같네요
prop으로 넘기면 CardWithDnd 같은 컴포넌트를 추가로 만들어서 사용하는 방식도 괜찮을 것 같아요!

const CardWithDnd = () => {

return (
  <Reorder.Item>
    <Card ReorderIcon />
  </Reorder.Item>
)
}
      <Reorder.Group values={items} onReorder={setItems} as="section">
        {items.map((item, idx) => idx < 3 ? (
          <Card />
        ) : (
          <CardWithDnd />
        ))
        }
      </Reorder.Group>

@sumi-0011
Copy link
Member Author

엇 Reorder.Group 내부에서도 요소들을 Reorder.Item인가 그걸로 감싸놓지 않으면 DND 영향 안받는걸로 아는데 아닌가유??

이부분을 확인해볼게요!
영향 안받으면 한번에 하면 좋을것 같네용! 감사합니다

@sumi-0011
Copy link
Member Author

엇 Reorder.Group 내부에서도 요소들을 Reorder.Item인가 그걸로 감싸놓지 않으면 DND 영향 안받는걸로 아는데 아닌가유??
DND 영향 안받는것 같아요! CardWithDndCard를 나누는 방향으로 하려구요 ㅎㅎ
영역 확인하기

1 similar comment
@sumi-0011
Copy link
Member Author

엇 Reorder.Group 내부에서도 요소들을 Reorder.Item인가 그걸로 감싸놓지 않으면 DND 영향 안받는걸로 아는데 아닌가유??
DND 영향 안받는것 같아요! CardWithDndCard를 나누는 방향으로 하려구요 ㅎㅎ
영역 확인하기

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented May 9, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 1e49962
Status: ✅  Deploy successful!
Preview URL: https://7433e76c.na-lab.pages.dev
Branch Preview URL: https://issue-48.na-lab.pages.dev

View logs

@github-actions github-actions bot added the test: unit Generate or update unit test label May 9, 2023
@codecov-commenter
Copy link

codecov-commenter commented May 9, 2023

Codecov Report

Patch coverage: 97.14% and project coverage change: +0.33 🎉

Comparison is base (d03b262) 97.22% compared to head (1e49962) 97.56%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #49      +/-   ##
==========================================
+ Coverage   97.22%   97.56%   +0.33%     
==========================================
  Files          15       25      +10     
  Lines          72      164      +92     
  Branches       11       28      +17     
==========================================
+ Hits           70      160      +90     
- Misses          2        4       +2     
Impacted Files Coverage Δ
src/features/createSurvey/cardList/ReorderIcon.tsx 66.66% <66.66%> (ø)
src/features/createSurvey/CreateSurvey.tsx 100.00% <100.00%> (ø)
src/features/createSurvey/cardList/Card.tsx 100.00% <100.00%> (ø)
src/features/createSurvey/cardList/CardList.tsx 100.00% <100.00%> (ø)
...features/createSurvey/cardList/CardListWithDnd.tsx 100.00% <100.00%> (ø)
src/features/createSurvey/cardList/CardWithDnd.tsx 100.00% <100.00%> (ø)
src/features/createSurvey/cardList/PlusIcon.tsx 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@sumi-0011 sumi-0011 marked this pull request as ready for review May 9, 2023 15:21
@sumi-0011 sumi-0011 linked an issue May 9, 2023 that may be closed by this pull request
Copy link
Member

@hyesungoh hyesungoh left a comment

Choose a reason for hiding this comment

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

의견 좀 남겨봤습니다 ... 라기엔 고봉밥을 지어버림 ... ㅋㅋㅋㅋㅋㅋㅋ

👍 👍 👍

테스트 코드 처음 작성하시는 것이였으면 힘드셨을 수도 있을 거 같은데 러닝커브 대박 👍

Comment on lines +1 to +10
const PlusIcon = () => {
return (
<svg width="27" height="26" viewBox="0 0 27 26" fill="none" xmlns="http://www.w3.org/2000/svg">
<path d="M26.2285 12.7266L0.774427 12.7266" stroke="#37C3FF" strokeWidth="2" />
<path d="M13.498 25.4551V0.000989047" stroke="#37C3FF" strokeWidth="2" />
</svg>
);
};

export default PlusIcon;
Copy link
Member

Choose a reason for hiding this comment

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

아이콘 같은 경우는 조금 common할 수 있을 거 같아요

이후에 디자이너 분들이 모아주시겠...죠??

그때는 옮기면 좋을 거 같아요 ! components/icon/plusIcon 같은 곳이랄까요 !!

Copy link
Member Author

Choose a reason for hiding this comment

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

넵 좋습니당!
아직은 어떻게 사용될지 몰라서 하위에 넣어두었어요

export default CardList;

const buttonStyle = css`
all: unset;
Copy link
Member

Choose a reason for hiding this comment

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

저도 unset으로 클리어 하는 걸 좋아하곤 하는데, normalize를 하지말고 전역으로 unset 해버릴까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

넵 onClick 이벤트를 넣으려고 button을 사용했는데, 기본으로 지정되어있던 border가 나오더라구요.
그래서 지정한 건데 전역적으로 지정되어있으면 편할 것 같습니다!


export default CardList;

const buttonStyle = css`
Copy link
Member

Choose a reason for hiding this comment

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

컨벤션 상으로 fooCss 처럼 Css 접미사를 붙이기로 했었던 거로 기억하는데 맞나요?? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

앗 그렇군요 추가하겠습니다

Comment on lines 28 to 36
css={css`
display: flex;
align-items: center;

padding: 26px 28px;

background-color: #eceef2;
border-radius: 12px;
`}
Copy link
Member

Choose a reason for hiding this comment

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

인라인 스타일링은 지양하는게 제 취향이긴 한데 어떠신가요 ㅋㅋㅋㅋ

같이 이야기해서 맞춰봐도 좋을 거 같아요 ~~

Copy link
Member Author

Choose a reason for hiding this comment

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

인라인 스타일링은 지양하는게 제 취향이긴 한데 어떠신가요 ㅋㅋㅋㅋ

같이 이야기해서 맞춰봐도 좋을 거 같아요 ~~

사실 저도.. 안좋아해요.
밑으로 뺄게요! 😆
제가 emotion을 사용하는건 처음이라 많이 알려주세요 ㅎㅎ

Comment on lines 38 to 52
<div
css={css`
user-select: none;
flex-grow: 1;
`}
>
<div
css={css`
font-size: 18px;
line-height: 25px;
color: #6b7180;
`}
>
{title}
</div>
Copy link
Member

Choose a reason for hiding this comment

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

물론 중요하지 않은 화면이라 생각하셔서 div만 사용하신 거 같은데,
조금 시맨틱하게 태그를 바꿔봐도 좋을 거 같은데 어떻게 생각하세요 ?! 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

물론 중요하지 않은 화면이라 생각하셔서 div만 사용하신 거 같은데, 조금 시맨틱하게 태그를 바꿔봐도 좋을 거 같은데 어떻게 생각하세요 ?! 🤔

좋아용ulli로 보여주는것이 좋을까요?

Copy link
Member

Choose a reason for hiding this comment

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

오오 리스트 태그 좋아요 !!

Copy link
Member

Choose a reason for hiding this comment

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

요거 텍스트도 div대신 폰트토큰을 지정해둔 Head1 Head2 Body1 Body2 컴포넌트를 만들어서 사용하는건 어떨까요?

const dragControls = useDragControls();

return (
<Reorder.Item className="dnd-item-component" value={item} as="div" dragListener={false} dragControls={dragControls}>
Copy link
Member

Choose a reason for hiding this comment

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

여기서 className이 하는 역할은 무엇인가요??


아하 더 읽어 봤는데, 테스트 때문인가요 ??

테스트 때문이시라면 testid를 이용하는 게 더 직관적일 거 같다고 생각해요! 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

여기서 className이 하는 역할은 무엇인가요??

아하 더 읽어 봤는데, 테스트 때문인가요 ??

테스트 때문이시라면 testid를 이용하는 게 더 직관적일 거 같다고 생각해요! 🤔

image `getByTestId`가 없어서 `className`으로 대체한 것이였는데, 제가 모르는 방법이 있을까요..? 🥺

Copy link
Member Author

Choose a reason for hiding this comment

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

const boardSquares = screen.getAllByTestId('.dnd-item-component');

요롷게 하니까 되네요! 해결했습니당 🙇

Comment on lines 46 to 56
<Reorder.Group
data-testid="dnd-component"
as="section"
values={items}
onReorder={setItems}
css={css`
display: flex;
flex-direction: column;
gap: 8px;
`}
>
Copy link
Member

Choose a reason for hiding this comment

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

👍 👍 👍 👍

src/features/createSurvey/cardList/Card.tsx Outdated Show resolved Hide resolved
Comment on lines 27 to 31
const knight = boardSquares[lastIndex - 1]?.firstChild;

if (!knight) {
throw new Error('knight is null');
}
Copy link
Member

Choose a reason for hiding this comment

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

knight가 의미하는 바가 무엇인가요??

Copy link
Member Author

Choose a reason for hiding this comment

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

그르게요? 열심히 찾다보니 성공했던거라
의미는 이해하고 있어서 이름 수정하겠습니다!

Copy link
Member

Choose a reason for hiding this comment

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

체스에서 다른 말을 뛰어넘을 수 있는 말이라서 knight를 사용하는 것 같네유

Copy link
Member

@sjoleee sjoleee left a comment

Choose a reason for hiding this comment

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

LGTM~

Comment on lines 38 to 52
<div
css={css`
user-select: none;
flex-grow: 1;
`}
>
<div
css={css`
font-size: 18px;
line-height: 25px;
color: #6b7180;
`}
>
{title}
</div>
Copy link
Member

Choose a reason for hiding this comment

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

요거 텍스트도 div대신 폰트토큰을 지정해둔 Head1 Head2 Body1 Body2 컴포넌트를 만들어서 사용하는건 어떨까요?

Copy link
Member

Choose a reason for hiding this comment

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

'cardList의 drag and drop 기능이 올바르게 동작하는지 확인한다' 에서, 드래그 앤 드롭 이후 정말로 데이터가 바뀌었는지 검증하는 내용이 필요할 것 같아요!

Copy link
Member Author

Choose a reason for hiding this comment

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

저도 그렇게 생각하는데요!
모르겠어요 ㅜㅜ

Comment on lines 27 to 31
const knight = boardSquares[lastIndex - 1]?.firstChild;

if (!knight) {
throw new Error('knight is null');
}
Copy link
Member

Choose a reason for hiding this comment

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

체스에서 다른 말을 뛰어넘을 수 있는 말이라서 knight를 사용하는 것 같네유

@sumi-0011
Copy link
Member Author

폰트 토큰은 혜성님 코드가 머지되면 수정하겠습니다!

@sumi-0011
Copy link
Member Author

드래그 앤 드롭 이후 정말로 데이터가 바뀌었는지 테스트하는거 도와주세요 🙏

@hyesungoh
Copy link
Member

드래그 앤 드롭 이후 정말로 데이터가 바뀌었는지 테스트하는거 도와주세요 🙏

흠 ... 테스트하기 쉬운 방식으로 CardList 구조를 바꾸던가..?

props를 활용해서 ... 다양한 상황에 대응할 수 있도록 한다던가

혹은 특정 이벤트 이후에 동작할 로직을 주입받아서 넘겨 받는 것을 확인한다던가 ... 하면 될 거 같은데 잘은 모르겠네요 저도 !

@sjoleee
Copy link
Member

sjoleee commented May 9, 2023

드래그 앤 드롭 이후 정말로 데이터가 바뀌었는지 테스트하는거 도와주세요 🙏

드래그 앤 드롭 이후에 boardSquares의 순서가 바뀌었는지 보는건 어떨까유??

Copy link
Member

@hyesungoh hyesungoh left a comment

Choose a reason for hiding this comment

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

👍 👍 👍 고생하셨어요 !!

@sumi-0011
Copy link
Member Author

sumi-0011 commented May 10, 2023

상태를 어떻게 트래킹하는지를 찾아봐야할 것 같은데 아직 잘 모르겠어서 ㅜㅜ
카드 순서가 바뀌는 테스트는 후에 추가하겠습니다 😓

@sjoleee
Copy link
Member

sjoleee commented May 12, 2023

상태를 어떻게 트래킹하는지를 찾아봐야할 것 같은데 아직 잘 모르겠어서 ㅜㅜ 카드 순서가 바뀌는 테스트는 후에 추가하겠습니다 😓

컴포넌트 내부의 상태를 트래킹하는 것은 쉽지 않을 것 같아요. 현재 컴포넌트가 데이터를 주입받는게 아니라 내부 데이터에 종속되어 있는데, 이걸 변경하면 테스트가 편해질 것 같아요!

ex)

  • CardList
    • Card들을 담아놓는 Dnd Wrapper로만 기능한다.
    • useState(각 Card에 대한 데이터 및 변경을 위한 setter)는 CardList에서 호출하지 않고, 외부로부터 주입받는다.
  • Card
    • prop에 맞게 뷰를 잘 띄워주는지만 테스트
  • CardWithDnd
    • Dnd 이벤트 이후 CardList가 들고있는 데이터가 변경되었는지 테스트한다

@sumi-0011
Copy link
Member Author

상태를 어떻게 트래킹하는지를 찾아봐야할 것 같은데 아직 잘 모르겠어서 ㅜㅜ 카드 순서가 바뀌는 테스트는 후에 추가하겠습니다 😓

컴포넌트 내부의 상태를 트래킹하는 것은 쉽지 않을 것 같아요. 현재 컴포넌트가 데이터를 주입받는게 아니라 내부 데이터에 종속되어 있는데, 이걸 변경하면 테스트가 편해질 것 같아요!

ex)

  • CardList

    • Card들을 담아놓는 Dnd Wrapper로만 기능한다.
    • useState(각 Card에 대한 데이터 및 변경을 위한 setter)는 CardList에서 호출하지 않고, 외부로부터 주입받는다.
  • Card

    • prop에 맞게 뷰를 잘 띄워주는지만 테스트
  • CardWithDnd

    • Dnd 이벤트 이후 CardList가 들고있는 데이터가 변경되었는지 테스트한다

CardList 구조를 변경했습니다!
Dnd 테스트는 실패했구요..
그 이외의 테스트는 작성해보았습니다!

export interface CardItemType {
title: string;
type: CardType;
id: number;
Copy link
Collaborator

Choose a reason for hiding this comment

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

id 가 props type으로 정의되어 있는데 사용하지 않고 있는 것 같아요! 물음표를 붙여도 좋을 것 같네용

// TODO : 서버 API 명세서 참고해서 type 변경
const handleNewItemAdd = () => {
setCardItems((prev) => {
const newId = prev.length;
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기서 newId는 클라에서 임의로 지정해줘도 괜찮을까요..? 서버에서 전달해주는 값이 있으면 그걸 사용하면 더 상태관리가 깔끔할 것 같다는 생각이 듭니다..!!

Copy link
Member Author

@sumi-0011 sumi-0011 May 14, 2023

Choose a reason for hiding this comment

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

이부분은 서버에서 데이터를 받아오는 부분이 없고, 나중에 서버로 데이터 보내기만 하면 되는 부분이여서
여기선 임의로 지정해줘도 괜찮을 것 같습니다!

서버에 보내줘야 하는 값는 id가 없어서 아예 id를 제거하는 방향도 생각중이예요

@sumi-0011 sumi-0011 merged commit 56876c7 into main May 16, 2023
@sumi-0011 sumi-0011 deleted the issue/48 branch May 16, 2023 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request test: unit Generate or update unit test
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Drag and Drop Component 만들기
5 participants