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] Calendar 페이지 기능 구현 #199

Merged
merged 17 commits into from
Nov 28, 2023

Conversation

ttaerrim
Copy link
Member

@ttaerrim ttaerrim commented Nov 27, 2023

설명

  • [FEATURE] 달력 페이지 구현 #201
  • 캘린더와 지도 페이지에서 사용할 mogacoAtom 전역 상태를 생성했습니다.
    • jotai는 하나의 카테고리로 분류할 수 있는 상태를 개별적으로 관리하는지 또는 객체로 묶어서 관리하는지 아직 학습 중입니다. 후자의 경우 리덕스 스타일이고, jotai와 redux의 철학이 상반되는 것으로 알고 있어 전자와 같은 방식으로 우선 작성했는데 잘 아시는 분 또는 의견 있으시면 코멘트 부탁드립니다.
      // 현재는 이렇게 작성되었음
      import { atom } from 'jotai';
      export const mogacoAtom = atom<string>('-1');
      
      // 이렇게 관리해야 하는 것인지?
      import { atom } from 'jotai';
      const initalState = {
        id: -1,
      };
      export const mogacoAtom = atom<{ id: number }>(initalState);
  • 첫 번째 설명과 약간 이어지는 부분으로, mocago 기본 id를 -1로 설정해 놓고 아무 이벤트(달력이나 지도)도 선택되지 않은 상태로 사용하려고 했습니다. useQuery의 enabled 옵션을 사용하여 id가 -1이 아닐 경우에만 API 요청을 하도록 설정할 수 있습니다. 그러나 캘린더 아이템을 선택하기 이전에 사이드바를 열 수 있어 [1] 이에 대한 처리가 필요합니다.
    @js43o 님이 주신 아이디어와 같이 선택 유도 메시지를 표시하도록 구현했습니다. [2]
  • 모각코 페이지 리스트 뷰의 경우 인원 수를 나타내는 필드가 없습니다. 사이드 바의 경우 인원 수를 나타내는 필드가 디자인 상으로 남아 있어 구현했는데, 어떤 이슈로 인해 /mogaco /:id GET API에서 현재 참여자 수 필드가 빠지게 되었는지 정리가 필요할 것 같습니다. 이에 따라 사이드바 또는 API 수정이 필요하거나 그대로 진행할 것 같습니다. @ldhbenecia @js43o
    • 알고 싶은 것
      1. /mogaco/:id/mogaco/:id/participant API가 분리된 이유
      2. `/mogaco/:id에서 participant 정보까지 받아오는 것은 힘들까요?
스크린샷 2023-11-27 17 48 00 스크린샷 2023-11-27 17 47 53

완료한 기능 명세

  • 캘린더 아이템과 사이드바 연결
  • 캘린더 API 변경된 명세에 맞게 participants 사용하도록 수정

스크린샷

기능 작업에 대한 스크린샷/화면 녹화 있을 경우 첨부하기

2023-11-27.17.43.02.mov

캘린더 아이템을 선택하기 이전에 사이드바를 열었을 경우 ⬇️⬇️⬇️

2023-11-27.17.44.02.mov

선택된 아이템 없을 경우 선택 유도 메시지 표시, 로딩 인디케이터 추가, 404 오류의 경우 재시도 메시지 표시

2023-11-28.15.37.26.mov

리뷰 요청 사항

특별히 리뷰해 주었으면 하는 부분, 고민되는 부분 기재하기

@ttaerrim ttaerrim self-assigned this Nov 27, 2023
@ttaerrim ttaerrim changed the title 🔨 달력 언어 한국어로 설정 [] Calendar Nov 27, 2023
@ldhbenecia
Copy link
Member

p2: 현재 참여자수는 원래부터 보내주지않았고 참여자 조회를 통해서 클라이언트에서 직접 추출해서 사용하는 것으로 알고 있습니다.

@ldhbenecia
Copy link
Member

ldhbenecia commented Nov 27, 2023

/mogaco/:id와 /mogaco/:id/participant API가 분리된 이유
-> 어떠한 모각코가 있으면 거기서 참가자를 전원 조회해야한다는 요청을 받아서 따로 팠습니다.

`/mogaco/:id에서 participant 정보까지 받아오는 것은 힘들까요?
-> 가능은 한데 너무 json 데이터가 길어지진 않을까요?, 가능합니다!

  • 제가 받았던 요청은 특정 게시물 조회 시 해당 게시물에 대한 방장을 끼워달라고 해서 그것을 처리했습니다.

Copy link
Collaborator

@js43o js43o left a comment

Choose a reason for hiding this comment

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

현재 모각코 리스트 페이지에서 각 아이템의 참여자 수를 추가로 표시하려면
아이템마다 id를 통해서 각각 /mogaco/:id/participants 요청을 보내주어야 할 것 같습니다. (Promise.all 이용?)

`/mogaco/:id에서 participant 정보까지 받아오는 것은 힘들까요?
-> 가능은 한데 너무 json 데이터가 길어지진 않을까요?, 가능합니다!

스키마를 바꾸는게 어렵지 않다면 저도 바꾸는 편이 더 좋을 것 같고, 너무 시간이 오래 걸린다 싶으면 그냥 인원 수 표시를 빼는 쪽으로 가도 좋습니다!

@ttaerrim
Copy link
Member Author

p2: 현재 참여자수는 원래부터 보내주지않았고 참여자 조회를 통해서 클라이언트에서 직접 추출해서 사용하는 것으로 알고 있습니다.
/mogaco/:id와 /mogaco/:id/participant API가 분리된 이유
-> 어떠한 모각코가 있으면 거기서 참가자를 전원 조회해야한다는 요청을 받아서 따로 팠습니다.

아하 확인했습니다!
혹시 그렇게 구현된 히스토리를 알 수 있을까요?
글 상세를 조회하는 데 한 번의 API 호출로 해결하는 것이 이상적이지 않을까 싶어 의견 드렸습니다.

@ldhbenecia
Copy link
Member

p2: 현재 참여자수는 원래부터 보내주지않았고 참여자 조회를 통해서 클라이언트에서 직접 추출해서 사용하는 것으로 알고 있습니다.
/mogaco/:id와 /mogaco/:id/participant API가 분리된 이유
-> 어떠한 모각코가 있으면 거기서 참가자를 전원 조회해야한다는 요청을 받아서 따로 팠습니다.

아하 확인했습니다! 혹시 그렇게 구현된 히스토리를 알 수 있을까요? 글 상세를 조회하는 데 한 번의 API 호출로 해결하는 것이 이상적이지 않을까 싶어 의견 드렸습니다.

아마 그 외관? 이라고 해야하나 거기서 리스트 별로 보여주는 부분을 구현할 때 이렇게 된 것 같습니다.

@ttaerrim
Copy link
Member Author

=> 회의를 거쳐 GET /mocago/:id에서 participant까지 보내 주도록 수정하기로 함

@ttaerrim ttaerrim changed the title [] Calendar [Feat] Calendar 페이지 기능 구현 Nov 27, 2023
@ttaerrim ttaerrim marked this pull request as ready for review November 27, 2023 10:09
@ttaerrim ttaerrim marked this pull request as draft November 27, 2023 10:12
Copy link
Collaborator

@js43o js43o left a comment

Choose a reason for hiding this comment

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

jotai는 하나의 카테고리로 분류할 수 있는 상태를 개별적으로 관리하는지 또는 객체로 묶어서 관리하는지 아직 학습 중입니다.

이것에 대해 조금 찾아보았는데, 각각의 상태를 atom으로 만들되 여러 atom을 객체로 묶은 것 자체도 atom으로 만들어 관리하는 방법을 권장하는 것 같습니다.
https://jotai.org/docs/guides/atoms-in-atom#storing-a-map-of-atom-configs-in-atom
이렇게 관리할 때의 장점은 비슷한 카테고리의 상태를 객체로 묶어 관리할 수 있으면서도 여러 atom 중 하나가 변경되더라도 다른 atom을 사용하는 뷰에 리렌더링을 일으키지 않는다고 하네요? Jotai가 바텀-업 방식이라는 게 이런 느낌인 것 같기도 합니다..

그러나 캘린더 아이템을 선택하기 이전에 사이드바를 열 수 있어 이에 대한 처리가 필요합니다.

'선택된 모각코 없음' 같은 UI로 간단하게 표시하고 기본값을 닫힘 상태로 두면 좋을 것 같습니다!
그리고 특정 모각코를 선택할 때에 자동으로 열리게 하고, 다른 곳을 클릭해서 선택을 해제하면 닫히도록 onSelect에서 setClosed를 호출해도 좋을 것 같아요
(모각코를 선택하지 않은 상태에선 사용자가 사이드바 열기 버튼을 눌러서 굳이 열지 않는 한 표시하지 않도록?)

@ttaerrim ttaerrim marked this pull request as ready for review November 28, 2023 08:13
@ttaerrim ttaerrim requested a review from js43o November 28, 2023 08:20
participantCount,
maxHumanCount,
address,
}: Pick<MogacoTypes, 'date' | 'maxHumanCount' | 'address'> & {
Copy link
Collaborator

Choose a reason for hiding this comment

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

p3: -Props로 따로 타입을 빼는 건 페이지 컴포넌트 한정일까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

굳이 분리할 필요가 있을까 싶어 이대로 작성했는데 가독성과 컨벤션 측면에서는 분리하는 것이 좋을 것 같네요. ed9a5dc 반영했습니다.

@@ -17,7 +17,13 @@ export const container = style({
padding: '2rem',
});
export const contents = style([sansRegular16, {}]);
export const group = style([sansBold12, {}]);
export const group = style([sansBold14, {}]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

p3: 빈 객체 {}를 추가한 이유는 추후 확장하게 될 상황을 고려하신 건가요?

Copy link
Member Author

Choose a reason for hiding this comment

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

그렇게 생각할 수도 있을 것 같고, 이전에 각각 스타일을 작성하다가 공통으로 뺄 수 있어서 수정하면서 그대로 남기게 되었습니다.

ddb6813에서 스타일 대신 폰트 스타일만 사용하도록 변경했습니다.

@ttaerrim ttaerrim merged commit c449fce into boostcampwm2023:develop Nov 28, 2023
1 check passed
@ttaerrim ttaerrim deleted the calender-improve branch November 28, 2023 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants