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

알림 ui 구현 및 api 연동 #751

Merged
merged 67 commits into from
Oct 18, 2023
Merged

알림 ui 구현 및 api 연동 #751

merged 67 commits into from
Oct 18, 2023

Conversation

chsua
Copy link
Collaborator

@chsua chsua commented Oct 15, 2023

🔥 연관 이슈

close: #740

📝 작업 요약

알림 ui 구현 및 api 연동.

⏰ 소요 시간

3일
생각보다 할일이 많았습니다..

🔎 작업 상세 설명

디자인 추가 / 변경

  1. 마이페이지 이동 트리거 변경
    • 기존 헤더에서 사이드바 닉네임 클릭으로 변경
    • 페이지 이동이란 것을 보여주기 위해 화살표 표시
    • 마이페이지 진입 시 화살표 사라짐 + 이벤트 사라짐
  2. 기존 마이페이지 이동 버튼 위치에 알림 아이콘 생성
    • 사용자 정보를 받아 활성화 표시 (프라이머리 색상으로)
  3. 알림 리스트
    • 툴팁을 만들어 데스크탑에서는 툴팁으로 표시
    • 모바일에서는 기존 drawer을 이용해 오른쪽에서 나오도록 표시
    • 기본적으로 토글로 컨텐츠와 신고를 오갈 수 있음
  4. 신고된 내역 알림을 클릭
    • 신고 상세 안내 페이지로 이동
    • 신고 관련 사유 및 정보를 가지고 있음
    • 기존 논의와 달리 게시물 페이지 전환이 아닌 게시물 포함으로 변경

기능 안내

  1. 알림 리스트 관련
// 데스크탑에서는 ToolTip 안에 AlarmContainer 가 children으로 들어감
// 모바일에서는 drawer 안에 AlarmContainer 가 children으로 들어감

AlarmContainer // 토글을 포함한 컴포넌트
 ┣ ContentAlarmList
 ┃ ┗ index.tsx
 ┣ ReportAlarmList
 ┃ ┗ index.tsx
 ┣ AlarmContainer.stories.tsx
 ┣ ListStyle.ts 
 ┣ index.tsx
 ┗ style.ts
  1. 포함되는 폴더
    • url에 따라 처리
      • alarm : 알림 리스트(컨텐츠/신고), 신고 알림 단건 조회, 알림읽기
      • user: 최신 알림 읽기

🌟 논의 사항

  1. preview 영상의 용량이 커서 동영상은 슬랙에 달아놓겠습니다
  2. 어디까지 범용성있게 가지고 가야 할 지 잘 모르겠습니다. 그래서 컴포넌트도 조금 혼란스러운 것 같아요. 예로, 리스트는 동일한 UI를 가지고 있지만 둘로 분리되어 있습니다. 서스펜스를 사용하기 위해 컴포넌트를 분리해 api를 넣기 위함입니다만 리스트는 따로 또 분리했어야 했나 하는 생각도 드네요.
  3. 왜인지 알림 리스트가 더보기를 누르지 않았는데도 자동으로 fetch를 해오는 경우가 있습니다. 왜인지 모르겠습니다. 더불어 페이지를 이동하고 돌아오면 fetch되어있던 이전 자료들이 눈에 보입니다. 이는 그 cashe time 때문인것같은데 그냥 0으로 설정할까요??
  4. 에러 처리는 아직 dev에 토스트 pr이 머지되지 않아서 하지 않았습니다. 404인 경우도 postDetail 404와 동일하게 처리할 예정입니다.
  5. 신고 처리 결과를 보는 트리거는 알림입니다만, 신고 처리 결과 자체가 알림과 관련이 없는데 api나 컴포넌트 이름에 alarm을 넣어야 하는지 의문이 듭니다. 고민하다가 사용자가 보는 라우터 url은 알림을 넣고 그 외에는 reportConfirmResult을 사용했습니다. 오히려 이게 더 헷갈린다고 하면 변경하겠습니다. 변수명 추천 받습니다.

- 배경을 누르면 툴팁 제거
- 툴팁 테두리색 변경
- 둥근 배경이 있는 경우에만 해당
- msw 데이터 불러오기 성공
- 더보기 클릭 시 추가 데이터 불러오기 성공
- 서스펜스, 에러바운더리, 로딩스피너 도입
- 사이드바에서 높이 설정
- 툴팁에서는 최대높이 및 가로길이 설정
- 비회원인 경우 알림 툴팁, 사이드가 안열리도록 조치
- 비회원인 경우 알림 툴팁, 사이드 버튼을 눌러도 api 통신되지 않도록 조치
Copy link
Member

@inyeong-kang inyeong-kang left a comment

Choose a reason for hiding this comment

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

Files changed 63..?
역대 프론트 Top3 안에 들거 같은데요..👍🎉

fe-리뷰완

@@ -28,12 +28,19 @@ describe('서버와 통신하여 유저의 정보를 불러올 수 있어야 한
expect(data).toEqual(transformUserInfoResponse(MOCK_USER_INFO));
});

test('클라이언트에서 사용하는 유저 정보 API 명세가 [nickname, gender, birthYear, postCount, voteCount]으로 존재해야한다', async () => {
test('클라이언트에서 사용하는 유저 정보 API 명세가 [nickname, gender, birthYear, postCount, voteCount, hasLatestAlarm]으로 존재해야한다', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

conflict 사전 방지를 위해 hasLatestAlarm 뒤에 role도 추가해주실 수 있나요...ㅎㅎㅎ

--gray: #F4F4F4;
--slate: #94a3b8;
--gray: #ced4da;
--bright-gray: #F4F4F4;
Copy link
Member

Choose a reason for hiding this comment

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

👍👍

margin-top: 10px;

//포함된 부분에서 이벤트 발생을 막는 속성
pointer-events: none;
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.

네네 그 포스트 컴포넌트를 넣는다면 onClick event를 막아도 방지가 되지 않고 투표가 됩니다😂
그래서 아예 css 속성으로 못하게 설정했습니다.

Comment on lines 44 to 46
{[...new Set(data.reason.map(each => REPORT_MESSAGE[each]))].map(reason => {
return <S.ListItem>{reason}</S.ListItem>;
})}
Copy link
Member

Choose a reason for hiding this comment

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

오 Set~~~~ 프로젝트에서 최초로 쓰였군요! 중복 제거 좋습니다👍

export default function ReportTargetPost({ postId }: { postId: number }) {
const { data } = usePostDetail(true, postId);

return data && <Post postInfo={data} isPreview={false} />;
Copy link
Member

Choose a reason for hiding this comment

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

기존 Post 컴포넌트 재사용 너무 좋아요!

@@ -8,6 +8,7 @@ export const BASE_PATH = {
SEARCH: '/search',
RANKING: '/ranking',
ANNOUNCEMENT: '/announcements',
ALARM: '/alarm',
Copy link
Member

Choose a reason for hiding this comment

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

alarms 로 복수형 형식으로 통일 어떠신가용


import * as S from './style';

// 70px = 토글 스위치의 크기(40px) 및 상하 padding(각 10px), gap(10px)
const alarmToolTipStyle: CSSProperties = {
Copy link
Member

Choose a reason for hiding this comment

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

CSSProperties 👍👍

@@ -38,7 +38,7 @@ export const Retry: Story = {
};

export const UserInfo: Story = {
Copy link
Member

Choose a reason for hiding this comment

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

오 유저정보 스토리인데 category가 'alarm'인가용??

@@ -18,7 +18,7 @@ export const LeftSideBar = () => {

return (
<div>
<NarrowMainHeader handleMenuOpenClick={openDrawer} />
<NarrowMainHeader handleCategoryOpenClick={openDrawer} handleAlarmOpenClick={() => {}} />
Copy link
Member

Choose a reason for hiding this comment

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

handleDrawerOpenClick 어떠신가요? (Drawer에 카테고리 외에 유저 프로필도 있어서...)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

근데 저 알림도 drawer라서 헷갈릴 것 같아요..

NICKNAME: '닉네임',
};

export default function ReportAlarmList({ closeToolTip }: { closeToolTip: () => void }) {
Copy link
Member

Choose a reason for hiding this comment

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

common 에 위치하기에는 도메인적 요소가 많아보이는데 common 외에 다른 곳에 위치시키는 것 어떠신가요?
수아가 생각하시는 common 폴더에 위치할 수 있는 기준이 궁금해요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

헤더 두 개에 사용될 예정이라 common에 넣었는데 만들고 보니 도메인이 강하네요!
수정하겠습니다!

@inyeong-kang
Copy link
Member

inyeong-kang commented Oct 17, 2023

🌟 논의 사항

preview 영상의 용량이 커서 동영상은 슬랙에 달아놓겠습니다

💯💯

어디까지 범용성있게 가지고 가야 할 지 잘 모르겠습니다. 그래서 컴포넌트도 조금 혼란스러운 것 같아요. 예로, 리스트는 동일한 UI를 가지고 있지만 둘로 분리되어 있습니다. 서스펜스를 사용하기 위해 컴포넌트를 분리해 api를 넣기 위함입니다만 리스트는 따로 또 분리했어야 했나 하는 생각도 드네요.

개인적인 의견으로 common 에는 NarrowHeader 처럼 여러번 범용적으로 재사용되는 것만 있어야 된다고 생각해요!

왜인지 알림 리스트가 더보기를 누르지 않았는데도 자동으로 fetch를 해오는 경우가 있습니다. 왜인지 모르겠습니다. 더불어 페이지를 이동하고 돌아오면 fetch되어있던 이전 자료들이 눈에 보입니다. 이는 그 cashe time 때문인것같은데 그냥 0으로 설정할까요??

🤔 오 우스 말대로 key 값이 동일해서 상태가 제대로 업데이트 안되는 것일수도 있겠군요!
혹시 cacheTime 0이면 제대로 업데이트되나요??

신고 처리 결과를 보는 트리거는 알림입니다만, 신고 처리 결과 자체가 알림과 관련이 없는데 api나 컴포넌트 이름에 alarm을 넣어야 하는지 의문이 듭니다. 고민하다가 사용자가 보는 라우터 url은 알림을 넣고 그 외에는 reportConfirmResult을 사용했습니다. 오히려 이게 더 헷갈린다고 하면 변경하겠습니다. 변수명 추천 받습니다.

github 처럼 approve 어떠신가요? reportApproveResult 추천합니다 (저도 신고 조치 fetch 함수명 reportApprove 로 바꿀까 고민 중..)

@chsua
Copy link
Collaborator Author

chsua commented Oct 17, 2023

fe-리뷰요청

Copy link
Collaborator

@Gilpop8663 Gilpop8663 left a comment

Choose a reason for hiding this comment

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

코드 변경된 부분 확인 후 어프로브 했습니다 🔥🔥🔥

Copy link
Member

@inyeong-kang inyeong-kang left a comment

Choose a reason for hiding this comment

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

충돌이 꽤 있는데 잘 해결되길...
코드 수정하신거 확인했습니다~

fe-리뷰완

@github-actions
Copy link

github-actions bot commented Oct 18, 2023

⚡️ Lighthouse report!

Category Score
🟠 Performance 62
🟠 Accessibilty 89
🟢 SEO 100
🟠 PWA 89
Category Score
🟢 First Contentful Paint 0.7 s
🟠 Largest Contentful Paint 3.5 s
🔴 Total Blocking Time 2,200 ms
🟢 Cumulative Layout Shift 0
🟢 Speed Index 3.3 s

@chsua chsua merged commit 88567a4 into dev Oct 18, 2023
1 check passed
@woo-chang woo-chang deleted the feat/#740 branch October 18, 2023 05:23
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.

알림 ui 구현 및 api 연동
3 participants