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

Migration@section/main page #83

Open
wants to merge 2 commits into
base: migration
Choose a base branch
from

Conversation

useruseruse
Copy link
Contributor

@useruseruse useruseruse commented Sep 18, 2024

Description

  • Main Page 를 functional component, typescript 로 마이그레이션 하였음.
  • get/notices 와 get/feed/{user}를 tanstack query를 이용하여 변경하였으며, useInfinitequery 로 무한스크롤을 구현하였음.
  • throttle 을 이용하여 스크롤 이벤트 핸들링을 2초마다 하도록 구현하였음.

Checklist

  • scroll event 를 통해서 스크롤을 감지하고 있는데 추후 interesect observer 로 변경하기
  • API 타입 패키지로부터 api 응답, 요청 등 타입 달아주기

Test

2024-09-17.9.52.50.mov

@useruseruse useruseruse added migrate migrate from JS CC to TS FC tanstack labels Sep 18, 2024
@useruseruse useruseruse self-assigned this Sep 18, 2024
Copy link
Member

@withSang withSang left a comment

Choose a reason for hiding this comment

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

좋아보입니다!! 고생 많으셨습니다.

  • user session 등이 react query와 redux 모두에서 관리되고 있습니다(App.jsx#L108-124). 이번 PR 말고 다음에 제거하셔도 좋겠다고 생각합니다.

Copy link
Contributor

@yumincho yumincho left a comment

Choose a reason for hiding this comment

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

merge하시기 전에 base migration으로 바꿔주시면 더 깔끔할 것 같습니다!

Comment on lines +33 to +34
export const useFeed = (user: User) => {
const userId = user?.id;
Copy link
Contributor

Choose a reason for hiding this comment

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

user를 param으로 넣어주는 대신 useSessionInfo 사용하면 어떨까요?

Suggested change
export const useFeed = (user: User) => {
const userId = user?.id;
export const useFeed = (user: User) => {
const { data: user } = useSessionInfo();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@withSang

  • 이부분은 그럼 user가 없을 때 처리 추가하기
    (기존과 동일하게 get session/info 의 status code 가 401 이면 user 를 null 로 설정하는 로직을 useUserSession의 query Fn 에 넣기)

  • 새로 작성된 컴포넌트에서는 redux store 에서가 아닌 useSessionInfo 를 통해서 user Session 가져오기 로 변경하면 어떨까요?

const MainPage: React.FC = () => {
const contentRef = useRef<HTMLDivElement | null>(null);

const { user } = useSelector((state: RootState) => state.common.user);
Copy link
Contributor

Choose a reason for hiding this comment

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

위에서 @withSang 님께서도 말씀해주셨는데, tanstack에서 관리하고 있는 user session을 사용하려면 이렇게만 작성해주시면 됩니다. 세션이 없을 때 처리가 (아마) 되어 있지 않아서 이번 PR에서 반영할지는 선택 사항..

Suggested change
const { user } = useSelector((state: RootState) => state.common.user);
const { data: user } = useSessionInfo();

Copy link
Contributor

Choose a reason for hiding this comment

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

머지는 안 되어 있는데, migration@sections/timetable 브랜치도 참고할 만할 거예요. TimetableTabs.tsx, timetable.ts에서 각각 userSessionInfo를 사용하고 있습니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#83 (comment)
위 스레드 참고.!
pr 분리해서 에러 처리 + 새로 작성 컴포넌트에 한해서 tanstack 으로 세션 가져오기 이렇게 바꾸면 될 거 같아용

src/pages/MainPage.tsx Outdated Show resolved Hide resolved
src/pages/MainPage.tsx Outdated Show resolved Hide resolved
src/pages/MainPage.tsx Outdated Show resolved Hide resolved
src/queries/main.ts Outdated Show resolved Hide resolved
Copy link

codecov bot commented Sep 18, 2024

Codecov Report

Attention: Patch coverage is 0% with 19 lines in your changes missing coverage. Please review.

Project coverage is 6.00%. Comparing base (6076b84) to head (d08a38f).

Files with missing lines Patch % Lines
src/queries/main.ts 0.00% 19 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           migration     #83      +/-   ##
============================================
+ Coverage       5.93%   6.00%   +0.07%     
============================================
  Files            214     214              
  Lines           6574    6490      -84     
  Branches        1745    1610     -135     
============================================
  Hits             390     390              
+ Misses          6023    6004      -19     
+ Partials         161      96      -65     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
migrate migrate from JS CC to TS FC tanstack
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants