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

모임상세, 모임 참여 API 연결 로직 구현 #79

Merged
merged 14 commits into from
Jul 22, 2024

Conversation

jaeml06
Copy link
Contributor

@jaeml06 jaeml06 commented Jul 19, 2024

PR의 목적이 무엇인가요?

2-2차 스프린트의 요구사항인 모임 상세, 모임 참여 API 연결 로직 구현

이슈 ID는 무엇인가요?

설명

모임 상세 페이지를 위한 모임 상세 API 연결 구현
모임 참여를 위한 모임 상세 API 연결 구현
이에 따른 MoimInfo 데이터 타입 변경

질문 혹은 공유 사항 (Optional)

@jaeml06 jaeml06 added FE 프론트엔드 관련 이슈입니다. 🌱 기능추가 feature (새로운 기능 구현) labels Jul 19, 2024
@jaeml06 jaeml06 requested review from ss0526100 and cys4585 and removed request for ss0526100 July 19, 2024 07:44
Copy link
Contributor

@ss0526100 ss0526100 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다~

tmpPage만 올려주시면 될 것 같아요

approve 드릴게용~

@@ -0,0 +1,16 @@
export const defaultHeaders = {
Copy link
Contributor

Choose a reason for hiding this comment

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

defaultHeader에 대해서 같이 조금 더 생각을 해봐야 할 것 같아요

저는 json이 아닌 다른 요청들도 필요할 수 있다고 생각이 들어요.(제가 잘 몰라서 그럼)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

넵 같은 의견입니다. 다만 현재는 header의 형태가 하나여서 이렇게 네이밍한것일 뿐 언제나 수정 가능합니다. 좋은 의견이네요

@@ -6,18 +6,20 @@ import {
} from '../../utils/formatters';
Copy link
Contributor

Choose a reason for hiding this comment

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

이렇게 작업 공간이 겹치는 파일이 있을 때(ex:api가 component를 건드는 경우) 어떻게 하면 좋을지 생각해봐야 할 것 같아요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

아마 merge하는 사람이 충돌을 해결해야 할 것 같은데 추후에 다시 의견을 나누어 보죠

@@ -1,6 +1,7 @@
const ROUTES = {
main: '/',
addMoim: '/add-moim',
detail: '/:moimId',
Copy link
Contributor

Choose a reason for hiding this comment

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

detail을 읽을때에 main 뒤에 바로 아이디를 넣는 건 생각을 조금 더 해봐야 할 것 같아요.

주소도 전역 공간이라고 생각하는데, 전역 공간을 해치는 느낌이 들어서요.

/moim-detail/:id 와 같이 바꾸는 건 어떨까하는 생각이 드네요

Copy link
Contributor

Choose a reason for hiding this comment

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

좋은 지점인 것 같아요.
개인적으로는 url이 mouda.com/moim/1 이렇게 표현되는게 적절해 보입니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

현재 moim/1과 같은 형태로 변경하였습니다.

@@ -12,6 +13,10 @@ const router = createBrowserRouter([
path: ROUTES.addMoim,
element: <MoimCreationPage />,
},
{
Copy link
Contributor

Choose a reason for hiding this comment

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

이 부분에 대한 커밋이 안올라온 것 같아요ㅜㅜ

다시 올려주시면 좋을 것 같아요.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

올렸습니다. 구현 자체랑 상관없어서 올리지 않았는데 올리는 것이 이해하는데 더 좋았겠네요

authorNickname: string;
description?: string;
}

export type MoimInputInfo = Omit<MoimInfo, 'moimId' | 'currentPeople'>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Omit 굿굿~~

Copy link
Contributor

@cys4585 cys4585 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다~
다음주에 코드 병합하면서 정리해보아요

Comment on lines +9 to +16
export const checkStatus = async (response: Response) => {
const statusHead = Math.floor(response.status / 100);
if (statusHead === 4 || statusHead === 5) {
const json = await response.json();
throw new Error(json.message);
}
return response;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

checkStatus 함수 분리, 네이밍 아주 좋네요~
근데 config라는 파일에 있는 게 어색한 것 같습니다 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

네이밍도 한번 고민해보아야 겠네요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

다만 header라는 것이 api통신을 위한 환경설정이라는 의미로 붙인 것이긴 합니다.

Comment on lines 20 to 21
export const getMoim = async (moimId: number): Promise<MoimInfo> => {
const url = `${ENDPOINTS.moims}/${moimId}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

const url = ${ENDPOINTS.moim}/${moimId};
이어야 할 것 같아요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

넵 수정했습니다

@@ -1,6 +1,7 @@
const ROUTES = {
main: '/',
addMoim: '/add-moim',
detail: '/:moimId',
Copy link
Contributor

Choose a reason for hiding this comment

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

좋은 지점인 것 같아요.
개인적으로는 url이 mouda.com/moim/1 이렇게 표현되는게 적절해 보입니다.

Comment on lines +11 to +16
onSuccess: () => {
queryClient.invalidateQueries({
queryKey: [QUERY_KEYS.moim],
});
onSuccess();
},
Copy link
Contributor

Choose a reason for hiding this comment

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

참여 신청이 성공했을 때, QUERY_KEYS.moim을 refetching 해야하는 이유가 있을까요??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

페이지에서 현재 참여 인원수를 반영하기 위해 refetching 하고 있습니다.

Comment on lines +6 to +9
const { data: moim, isLoading } = useQuery({
queryKey: [QUERY_KEYS.moim, moimId],
queryFn: () => getMoim(moimId),
});
Copy link
Contributor

Choose a reason for hiding this comment

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

queryKey를 두 개를 두면 nesting이 되는 걸까요?? 꿀팁 ㄱㅅㄱㅅ~

@@ -26,7 +27,7 @@ export default function MoimCreationPage() {

return (
<FormLayout>
<FormLayout.Header onBackArrowClick={() => navigate(ROUTES.main)}>
<FormLayout.Header onBackArrowClick={() => navigate(-1)}>
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

authorNickname: string;
description?: string;
}

export type MoimInputInfo = Omit<MoimInfo, 'moimId' | 'currentPeople'>;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

headers: defaultHeaders,
};

export const checkStatus = async (response: Response) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

공통 API 에러처리 분리

@@ -6,10 +6,14 @@ const createQueryClient = () => {
queries: {
networkMode: 'always',
},
mutations: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

queryClient 에러 처리 추가

Comment on lines +11 to +16
onSuccess: () => {
queryClient.invalidateQueries({
queryKey: [QUERY_KEYS.moim],
});
onSuccess();
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

페이지에서 현재 참여 인원수를 반영하기 위해 refetching 하고 있습니다.

Comment on lines 20 to 21
export const getMoim = async (moimId: number): Promise<MoimInfo> => {
const url = `${ENDPOINTS.moims}/${moimId}`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

넵 수정했습니다

@@ -12,6 +13,10 @@ const router = createBrowserRouter([
path: ROUTES.addMoim,
element: <MoimCreationPage />,
},
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

올렸습니다. 구현 자체랑 상관없어서 올리지 않았는데 올리는 것이 이해하는데 더 좋았겠네요

@@ -1,6 +1,7 @@
const ROUTES = {
main: '/',
addMoim: '/add-moim',
detail: '/:moimId',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

현재 moim/1과 같은 형태로 변경하였습니다.

@@ -0,0 +1,16 @@
export const defaultHeaders = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

넵 같은 의견입니다. 다만 현재는 header의 형태가 하나여서 이렇게 네이밍한것일 뿐 언제나 수정 가능합니다. 좋은 의견이네요

@@ -6,18 +6,20 @@ import {
} from '../../utils/formatters';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

아마 merge하는 사람이 충돌을 해결해야 할 것 같은데 추후에 다시 의견을 나누어 보죠

Comment on lines +9 to +16
export const checkStatus = async (response: Response) => {
const statusHead = Math.floor(response.status / 100);
if (statusHead === 4 || statusHead === 5) {
const json = await response.json();
throw new Error(json.message);
}
return response;
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

네이밍도 한번 고민해보아야 겠네요

Comment on lines +9 to +16
export const checkStatus = async (response: Response) => {
const statusHead = Math.floor(response.status / 100);
if (statusHead === 4 || statusHead === 5) {
const json = await response.json();
throw new Error(json.message);
}
return response;
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

다만 header라는 것이 api통신을 위한 환경설정이라는 의미로 붙인 것이긴 합니다.

@jaeml06 jaeml06 merged commit 0903adc into develop-frontend Jul 22, 2024
@jaeml06 jaeml06 deleted the feat/#67 branch July 22, 2024 05:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FE 프론트엔드 관련 이슈입니다. 🌱 기능추가 feature (새로운 기능 구현)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants