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

feat: [LINKER-106] Dropdown 컴포넌트 개발 #53

Merged
merged 17 commits into from
Feb 14, 2024
Merged

Conversation

JjungminLee
Copy link
Collaborator

@JjungminLee JjungminLee commented Feb 8, 2024

작업 내용

  • 플로팅버튼 컴포넌트 개발을 했습니다!
  • rightAddon값을 아이콘 또는 elipse로 자유롭게 넣을 수 있도록 개발해봤어요! (우성님이 만들어주신 List component를 참고했습니다!)
  • 빠르게 어프룹 해주시면 다음 작업인 타임라인 수정하기, 삭제하기 작업을 할 수 있을것 같아용!

알아두시면 좋아요!

  • 현재 API 필드값이 달라서 주석 처리해놓은 부분이 많아요! 빨간줄 뜨더라도 그냥 무시해주세용!
  • 혹시 서버컴포넌트에서 문제가 생긴다면 디코로 꼭 알려주세요!

반영 화면

KakaoTalk_Video_2024-02-09-08-40-56.mp4

내일 추가적으로 할 일 (안까먹으려고 쓰는!)

  • 플로팅 버튼 아이템별로 뜨는 문제 해결
  • 일정상세쪽에도 플로팅 버튼 추가

@JjungminLee JjungminLee self-assigned this Feb 8, 2024
);
};

export default Object.assign(Floating, { Item: FloatingItem });
Copy link
Member

Choose a reason for hiding this comment

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

UI 컴포넌트에서 사용하는 Floating 용도와는 다른 컴포넌트 같아요!

Select 또는 Dropdown이 어떨까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

오 DropDown 좋아용! 이렇게 수정할게요!

Comment on lines 17 to 23
<div>
{floatingType === 'SCHEDULE' ? (
<div className={clsx(container, bottomSpacing.scheduleDetail)}>{children}</div>
) : (
<div className={clsx(container, bottomSpacing.timeline)}>{children}</div>
)}
</div>
Copy link
Member

@useonglee useonglee Feb 9, 2024

Choose a reason for hiding this comment

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

2rem 차이로인해 type을 넣어서 분기치기 보다는 사용처에서 className을 넣어주는게 더 좋을 것 같아요
type이 두개로 정해져있으면 확장성이 없어서 다양한 케이스에서 사용하기 힘들 것 같습니닷

interface BaseProps extends HTMLAttributes<HTMLButtonElement> {
  children: ReactNode;
  className?: string;
}

const Floating = ({ children, className }: BaseProps) => {
  return (
     <div className={clsx(container, className)}>
       {children}
     </div>
  )
};



// Timeline.tsx
<Floating className={timelineFloating} />

right: '0.2rem',
display: 'inline-block',
borderRadius: '1.6rem',
background: colors.white,
Copy link
Member

Choose a reason for hiding this comment

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

background: colors.gray000

현재 디자인쪽에 다크모드 색상 작업이 안되어있지만, 위에 처럼 사용하면 추후에 다크모드가 알아서 적용됩니닷 🙏

const [time, setTime] = useState('');
const [kebabClick, setKebabClick] = useState(false);
Copy link
Member

Choose a reason for hiding this comment

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

const [isOpenFloating, setIsOpenFloating] = useState(false);

kebabClick 이라는 것이 직관적이지 않은 것 같아요. kebab이 무엇인지 찾아보게 되는 것 같네용

@@ -54,4 +55,4 @@ const Button = forwardRef<HTMLButtonElement, Props>(
},
);

export default Object.assign(Button, { FAB, KakaoLogin });
export default Object.assign(Button, { FAB, KakaoLogin, Floating });
Copy link
Member

Choose a reason for hiding this comment

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

버튼과 별도로 분리되어도 좋을 것 같아요! 목적이 버튼과는 다른 것 같아서용

@@ -12,7 +12,7 @@ interface Props extends Omit<HTMLAttributes<HTMLButtonElement>, 'type'> {
}
// 플로팅 버튼의 두가지 종류 -> 오른쪽이 아이콘, 오른쪽이 원
Copy link
Member

Choose a reason for hiding this comment

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

이 주석은 제거해도 될 것 같아요


import FloatingItem from './DropdownItem';

interface BaseProps extends Omit<HTMLAttributes<HTMLButtonElement>, 'type'> {
Copy link
Member

Choose a reason for hiding this comment

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

Omit은 제거해도 될 것 같아유

return <div className={className}>{children}</div>;
};

export default Object.assign(Dropdown, { Item: FloatingItem });
Copy link
Member

Choose a reason for hiding this comment

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

FloatingItem -> DropdownItem

Comment on lines 149 to 182
<button onClick={handleDropdownClick}>
<Icon name="down" size={20} />
</button>
{dropdownClick ? (
<div>
<Dropdown className={dropdownContainer}>
<Dropdown.Item
text="개인일정"
onClick={handleCalendarToggleClick}
rightAddon={
<div
className={clsx(
scheduleCalendarDropDownElipse,
calendarElipseColor.personal,
)}
></div>
}
></Dropdown.Item>
<div className={dropdownDivider}></div>
<Dropdown.Item
text="생일"
onClick={handleCalendarToggleClick}
rightAddon={
<div
className={clsx(
scheduleCalendarDropDownElipse,
calendarElipseColor.birthday,
)}
></div>
}
></Dropdown.Item>
</Dropdown>
</div>
) : null}
Copy link
Member

Choose a reason for hiding this comment

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

Dropdown을 쓸 때 마다 dropdownClick 이라는 상태값을 만들어줘야하고 button 컴포넌트도 매번 만들어줘야 하는데, Dropdown과 별도로 반복되는 구조가 많네용

Dropdown이라는 컴포넌트안에 상태값 + 버튼 인터페이스를 모두 제공하는 구조로 가보는건 어떨까요?

https://www.radix-ui.com/primitives/docs/components/dropdown-menu

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image

제가 제대로 이해한건지 모르겠어서 다시 여쭤보는데요..! 이 구조 처럼 check된 상태인건지 + 이벤트함수+버튼 같이 넣어보자는거죵?!

Copy link
Member

Choose a reason for hiding this comment

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

말씀해주신건 Checkbox 영역이라 리뷰드린 내용과는 다른 것 같아요!

저는 Trigger 버튼을 말씀드린거였습니닷
스크린샷 2024-02-10 오후 3 04 47

제가 구현해놓은 Modal도 Trigger 버튼을 만들어놔서 참고해보셔도 좋을 것 같아요!

매번 button과 상태값을 만들필요없이 Modal을 열 수 있도록 구현해놨습니다

Copy link
Collaborator Author

@JjungminLee JjungminLee Feb 11, 2024

Choose a reason for hiding this comment

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

제가 어제 이쪽 보다가 하루종일 헤맸었는데요..! 우선 더는 늘어지면 안될것 같아 이전코드로 다시 원복시키고 다음작업들을 진행중입니당! 하지만 Trigger써서 한번 해보고 싶긴해서 몇가지 질문을 드려요! 우선 contextAPI를 사용하는것 같길래 저도 우성님 처럼 Dropdown/context.ts를 만들어뒀습니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

드롭다운의 경우 타임라인 아이템 마다 더보기 버튼을 누르면 수정/삭제 드롭다운이 떠야하며(적어도 3개 이상의 아이템마다 드롭다운이 붙음), 일정 상세에서도 캘린더 컴포넌트의 드롭다운을 누르면 개인일정인지/생일인지 선택할 수 있는 드롭다운이 있어야 (단일드롭다운) 후자의 경우에는 우성님이 만들어주신 Modal.tsx의 로직을 참고하면 해볼수는 있을것 같은데 전자의 경우가 좀 어렵더라구요 . 타임라인쪽에서 드롭다운을 사용하려면 아이템이 엄청 여러개 있고 그중 하나를 클릭했을때만 드롭다운이 나와야하는데 contextAPI를 쓰면 어떤 아이템을 클릭했는지 판별하려면 어떤 코드를 더 추가해야하는지 감이 안와요. context string넣는곳에 id를 따로 추가해야하는건가 하는 생각도 들고요..ㅜㅜ 혹시 해결방법이 있을까요?!

Copy link
Member

Choose a reason for hiding this comment

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

타임라인에서만 open 상태를 넘겨줘야겠네요. id 추가할 필요없이 open 상태 넘겨줄 때 조건식으로 넘겨주면 좋을 것 같아요!

// schedules.tsx
const [listId, setListId] = useState(0);
const [isOpen, setIsOpen] = useState(false);

<Dropdown open={listId === listIndex && isOpen}>
  <Dropdown.Trigger>
    // 더보기 클릭
    <Icon name="more" />
  </Dropdown.Trigger>

  // ..생략
</Dropdown>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

얘도 사실상 Id없어도 되더라구요! 헤드리스 컴포넌트 패턴 신통방통 하네여

Copy link
Member

Choose a reason for hiding this comment

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

고생많으셨어요!! 👍👍👍

};
const handleDeleteClick = () => {
console.log('삭제하기 클릭');
deleteDataQuery.mutate(scheduleId);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@useonglee 원래 mutate하면 실행되어야하는거 아닌가용??!!

Copy link
Member

Choose a reason for hiding this comment

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

delete api 요청은 잘 됐나요!?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

넵 이거 그냥 ky로만 호출했을때는 잘됐었어요!!

Copy link
Member

Choose a reason for hiding this comment

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

mutate의 mutationFn은 잘 동작하고, onSuccess 부분만 실행이 안된거죠!?
그렇담 이것도 query key 해결되는대로 같이 봐얄듯 싶네용

return useMutation({
mutationFn: (scheduleId: number) => deleteSchedule(scheduleId),
onSuccess: (_, scheduleId: number) => {
queryClient.invalidateQueries({ queryKey: ['schedule', scheduleId] });
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@useonglee scheduleId에 해당하는 쿼리키를 제거해서 유저가 삭제후 굳이 새로고침을 하지 않아도 다시 화면 렌더링이 되게 하고 싶었는데요..! 안되더라구요 ㅜㅜㅜ 😭 이러려면 애초에 get API로 조회할때부터 그때부터 쿼리키를 달아줬어야 하는지 궁금합니다..!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image

Copy link
Member

Choose a reason for hiding this comment

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

넵넵 react-query는 key 기반으로 캐시 관리를 하고 있어서 queryClient.invalidateQueries 메서드도 query key를 찾고, 해당 query key 캐시에 해당하는 데이터를 stale 상태로 바꾸는 것으로 알고 있어요

schedule 데이터를 조회(GET)할 때 useSuspenseQuery 사용 후 동일한 query key를 사용하면 잘 될 것 같아보이는데, 시도해보시는거 어떠신가요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

오호 !! 해보겠습니당! 감사해요 ㅜㅜㅜㅜㅜ

@useonglee useonglee changed the title feat: [LINKER-106] 플로팅버튼 컴포넌트 개발 feat: [LINKER-106] Dropdown 컴포넌트 개발 Feb 14, 2024
@useonglee useonglee merged commit 3f9b74a into develop Feb 14, 2024
6 checks passed
@useonglee useonglee deleted the feat/LINKER-106 branch February 14, 2024 07:45
Comment on lines +33 to +35
// if (query === '') {
// setQuery(input);
// }
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
Collaborator Author

Choose a reason for hiding this comment

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

여기 검색이 좀 꼬여서 다음작업에서 다 지워둘게요! 아마 안사용할것 같습니당!!


return <TimelineSearch schedules={timelineSearchData.schedules} />;
return <>{data && <TimelineSearch schedules={data?.schedules} />}</>;
Copy link
Member

Choose a reason for hiding this comment

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

useQuery -> useSuspenseQuery를 사용하면 옵셔널 체이닝 + 널병합을 사용하지 않고

return (
  <TimelineSearch schedules={data.schedules} />
)

요렇게 사용할 수 있어요

Comment on lines +20 to +22
router.push(
`/my/timeline/search?from=${`${format(date, 'yyyy-MM-dd')} 00:00:00`}&to=${`${format(date, 'yyyy-MM-dd')} 11:59:59`}&limit=32`,
);
Copy link
Member

Choose a reason for hiding this comment

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

00:00:00처럼 시간도 붙여줘야 하나요? 불 필요해보여서요 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이거 백엔드에서 00:00:00까지 받아서 저렇게 붙여놓은건데 우선 라우터로 푸쉬하는 방식을 쓰지 않을것 같아요! 그냥 서버컴포넌트만 사용하면 라우터 푸쉬를 해서 url에서 날짜,시간 데이터를 받았는데요! 리액트 쿼리 써서 client state에서 전부 관리하려구요! 라우터 이동 없이용!


const useGetNearSchedule = () => {
return useQuery<GetTimelineRes>({
queryKey: ['schedule'],
Copy link
Member

Choose a reason for hiding this comment

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

위에도 queryKey가 schedule인게 있던데, 유니크하게 api url로 query key를 가져가는건 어떠신가요!?

추후에 쿼리키가 꼬여서 예상치 못한 곳에서 캐시되는 순간 디버깅하기가.. 굉장히 힘듭니닷
(tanstack devtools를 깔지 않았어요)

queryKey: ['/api/v1/schedules/near-term']

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

헉 네넵 바꿔두겠습니당!!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

삭제 했을때 바로 반영이되려면 near-term API랑 일정삭제 API의 쿼리키가 같아야 하더라구요. API url을 쿼리키로 하는게 맞나?!라는 고민이 들어요..!

Copy link
Member

Choose a reason for hiding this comment

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

useQuery를 사용하는 queryKey 말씀드린거였어용
말씀해주신 삭제는 useMutation에서 queryKey 기반으로 query를 찾으니, 같아야 하는게 맞습니다!

['schedule'] <- 이게 현재는 schedule 관련 api가 몇 개 없다보니 괜찮은데, 경우의 수가 늘어나고 추후에 다른 개발자가 해당 queryKey를 인지하지 못하고 똑같이 사용했을 때를 생각하고 말씀드린 거였어요!

그리고 queryKey를 사용처마다 하드 코딩해주셨는데, 메서드로 만들어서 관리하면 좋을 것 같아요

useGetNearSchedule.getKey = () => {
 return ['/api/v1/schedules/near-term']
}

queryKey: useGetNearSchedule.getKey();

// mutation
queryClient.invalidateQueries({ queryKey: useGetNearSchedule.getKey() })

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아하 네넵 이해했습니다! 그럼 알려주신 방식대로 메서드로 관리해볼게요! 감사해용!

{member &&
member.map((item) => (
<div key={item.memberId}>
{contacts &&
Copy link
Member

Choose a reason for hiding this comment

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

contacts.length > 0 또는 contacts != null 처럼 boolean 값이 오면 좋을 것 같아요

https://ko.legacy.reactjs.org/docs/conditional-rendering.html

Comment on lines +16 to +24
const Dropdown = ({ children, className }: Props) => {
const [isOpen, setIsOpen] = useState(false);

return (
<DropdownProvider isOpen={isOpen} onOpenChange={() => setIsOpen((prev) => !prev)}>
<button className={className}>{children}</button>
</DropdownProvider>
);
};
Copy link
Member

Choose a reason for hiding this comment

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

open 상태값을 사용하는 도메인(service/web)에서 가지고 있어야할 경우도 있기 때문에 props로 넘겨주는 경우도 있으면 좋을 것 같아요

https://github.com/YAPP-Github/23rd-Web-Team-1-FE/blob/develop/packages/lds/src/Modal/Modal.tsx

구현해놓은 Modal 컴포넌트 참고차 링크 남겨놓을게요

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants