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

header alarm 구현완료 #102

Open
wants to merge 24 commits into
base: develop
Choose a base branch
from
Open

header alarm 구현완료 #102

wants to merge 24 commits into from

Conversation

live-small
Copy link
Member

@live-small live-small commented Sep 13, 2022

개요

유저에게 온 알람을 보여줌

작업사항

  • header의 alarm api 연결 -> UI 구현
  • 무한스크롤 구현
    : 마지막 요소가 보이면 alarm api를 호출하니까, 쓰기 불편하더라구요. (데이터 로드해오는 시간에 의해 끊김 -> 무한스크롤의 장점을 활용하지 못하는 느낌?) 그래서 영인님처럼 마지막에서 4번째 전 요소가 api 트리거 할 수 있도록 구현해뒀습니다.

주요 변경 로직

  • useOnView
    : useEffect의 ref -> ref.current로 바꿨습니다
    이유 ref.current가 바껴도 ref의 주소값은 동일하기때문에 useEffect가 실행되지 않는 케이스가 생겨 바꾸게 되었습니다.

참고

팔로잉 모달은 아직 미완성인데, pr이 너무 커질 거 같아서 쪼개서 보냅니다! => 팔로잉 모달까지 완료 ! (10.03)

@live-small live-small self-assigned this Sep 13, 2022
@live-small live-small added the enhancement New feature or request label Sep 13, 2022
Copy link
Collaborator

@kimyoungyin kimyoungyin left a comment

Choose a reason for hiding this comment

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

밖이라 나중에 더 리뷰 남길게요! 일단 아무것도 없을 떄 레이아웃도 구현하면 좋을 것 같아요
스크린샷 2022-09-17 오후 3 15 33

src/@type/index.d.ts Outdated Show resolved Hide resolved
src/@type/index.d.ts Outdated Show resolved Hide resolved
src/components/Common/Header/alarm/utils.ts Show resolved Hide resolved
alarmContainerRef: React.RefObject<HTMLDivElement>;
}) {
const dispatch = useAppDispatch();
const [pageToLoad, setPageToLoad] = useState(1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

전역 state를 사용하실 거라면 혹시 pageToLoad만 전역 state에서 제외하신 이유가 있을까요? alarmSlice 안에 넣으면 thunk를 dispatch할 때도 인자도 넣을 필요도 없어 편할 것 같아요.

Copy link
Member Author

Choose a reason for hiding this comment

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

pageToLoad를 꼭 전역 상태값으로 두어야할 이유가 뭘까용? 컴포넌트 안에서 처리가능하다면 컴포넌트 안의 상태값을 이용하자! 라는 생각이라서 이렇게 구현했습니다!

Copy link
Collaborator

Choose a reason for hiding this comment

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

alarmSlice에 totalPagespageToLoad에 대해 이해가 잘 안되서 설명 부탁드려도 될까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

pageToLoad는 불러올 페이지 번호,
totalPage는 총 페이지 수,
alarmList는 현재 보여지는 alarm리스트입니다!

로직은, pageToLoad로 알람의 특정 페이지를 불러와서 alarmList에 넣습니다.
이때, totalPage보다 큰 경우엔 더 불러올 페이지가 없으니까, 알람 api 호출하지 않도록 했어요!

이전엔 pageToLoad 빼고 다른 상태값을 다 전역에서 관리했던데..
제가 왜 이렇게 구현한지(?) 이해가 안가더라구요??ㅋㅋㅋ

해당 상태값 모두 컴포넌트 내에서 관리하도록 수정했습니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

@kimyoungyin 아래 커밋 봐주시면 됩니다!
54e36fc

Comment on lines 22 to 23
extraReducers: (build) => {
build //
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기 팔로우/언팔로우 관련 extraReducer 추가 예정인가요?

Copy link
Member Author

Choose a reason for hiding this comment

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

아뇨! 팔로우/언팔로우 api는 home thunk에 있는 로직 재사용했습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

아 넵 근데 promise 상태를 보여주려면 extraReducer가 필요할 수 있을지도 모르겠네요. 아래 리뷰 참고 부탁드립니다.

src/components/Common/Header/NavItems.tsx Outdated Show resolved Hide resolved
@live-small
Copy link
Member Author

@kimyoungyin 영인님 알람 작업 끝냈습니당

Comment on lines -11 to -13
import { ReactComponent as Map } from "assets/Svgs/map.svg";
import { ReactComponent as MapActive } from "assets/Svgs/map-active.svg";

Copy link
Member Author

Choose a reason for hiding this comment

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

안쓰는 이미지라서 삭제했습니다 @kimyoungyin
아마 저희가 저 기능 구현안하기로해서 작업안한거 맞죠..?

Copy link
Member Author

Choose a reason for hiding this comment

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

@kimyoungyin 영인님 이 부분도 봐주신거 맞나용?

@live-small live-small changed the title header alarm UI, 무한스크롤 구현 header alarm 구현완료 Oct 4, 2022
Copy link
Collaborator

@kimyoungyin kimyoungyin left a comment

Choose a reason for hiding this comment

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

늦게 리뷰 남겨드려서 죄송합니다 ㅠ
최대한 자세히 보고 리뷰 남겨드려요!

(요청)
아 그리고 몇몇 파일들 공통 사항인데 갑자기 파일명이 카멜 케이스가 아니라 스네이크 케이스로 작성되어있던데 통일성을 위해 수정 부탁드립니다!

src/@type/index.d.ts Outdated Show resolved Hide resolved
src/@type/index.d.ts Outdated Show resolved Hide resolved
src/@type/index.d.ts Outdated Show resolved Hide resolved
Comment on lines 84 to 86
<Button onClick={followHandler} style={{ height: "30px" }}>
팔로우
</Button>
Copy link
Collaborator

Choose a reason for hiding this comment

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

팔로우 요청 중일 때 로딩 circle이 필요할 것 같은데 관련 state를 추가해야 할 것 같은데 어떠세요?
pending 상태가 아니라면 버튼을 비활성화 시키는 게 좋을 것 같습니다

Comment on lines 40 to 45
const followHandler = () => {
dispatch(postFollow({ username: alarm.agent.username })) //
.then(() => {
setIsFollowing(!isFollowing);
});
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

에러가 발생했을 때는 어떻게 처리할까요? 아래 버튼 관련해서 개선하면 좋을 것 같아요.

참고로 homeSlice에는 각 게시글에 followLoading state가 있긴 합니다

Comment on lines 22 to 23
extraReducers: (build) => {
build //
Copy link
Collaborator

Choose a reason for hiding this comment

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

아 넵 근데 promise 상태를 보여주려면 extraReducer가 필요할 수 있을지도 모르겠네요. 아래 리뷰 참고 부탁드립니다.

alarmContainerRef: React.RefObject<HTMLDivElement>;
}) {
const dispatch = useAppDispatch();
const [pageToLoad, setPageToLoad] = useState(1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

alarmSlice에 totalPagespageToLoad에 대해 이해가 잘 안되서 설명 부탁드려도 될까요?

Comment on lines +85 to +88
const alarmContainerRef = useRef<HTMLDivElement | null>(null);
const alarmModalControllerRef = useRef<HTMLSpanElement | null>(null);
useOutsideClick(alarmContainerRef, setIsAlarmOn, alarmModalControllerRef);

Copy link
Collaborator

Choose a reason for hiding this comment

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

이 두 Ref는 useOutsideClick을 위해 생성한 걸까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

네네 맞아요

@kimyoungyin kimyoungyin self-requested a review November 14, 2022 12:52
파일명, 타입모듈명, type -> interface
전역, 컴포넌트로 섞여있었음 -> 컴포넌트에서만 관리
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants