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

[안상균] sprint12 #744

Open
wants to merge 18 commits into
base: React-안상균
Choose a base branch
from
Open

[안상균] sprint12 #744

wants to merge 18 commits into from

Conversation

emotigom
Copy link
Collaborator

@emotigom emotigom commented Jul 19, 2024

요구사항

기본

주요 변경사항

  • react query 적용

스크린샷

멘토에게

  • 조금밖에 못했어요.. 늦었지만 최대한 해보겠습니다~^^
  • 셀프 코드 리뷰를 통해 질문 이어가겠습니다.

@emotigom emotigom requested a review from lisarnjs July 19, 2024 16:15
@emotigom emotigom changed the title 안상균[sprint12] [안상균] sprint12 Jul 19, 2024
@emotigom emotigom added 매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. 미완성🫠 죄송합니다.. labels Jul 19, 2024
Copy link
Collaborator

@lisarnjs lisarnjs left a comment

Choose a reason for hiding this comment

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

상균님 실력 너무 좋은데요!?
기본적인 코드에 센스가 있으신가봐요 👍
이번 한 주도 화이팅입니다!
ps. 겹치는 내용이 있다면 리뷰는 한번만 쓰입니다 :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

파일명도 같이 바꿔주시는 거 잊으신걸까요!? -> handler.ts
아, 테스트용 일까요? ㅎㅎ

Copy link
Collaborator

Choose a reason for hiding this comment

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

전체적으로 개선할 수 있는 방법을 코드로 보여드리자면 요런 느낌이 될 수 있겠네요!
fetchFromApi()라는 함수로 재사용성 있게 만들어서 그 뒤에 불러올 api 요청들을 함수 하나로 끝낼 수 있게 됩니다!

const API_BASE_URL = "https://panda-market-api.vercel.app";

interface FetchParams {
  [key: string]: string | number | boolean;
}

async function fetchFromApi(endpoint: string, params?: FetchParams) {
  const url = new URL(`${API_BASE_URL}/${endpoint}`);
  if (params) {
    url.search = new URLSearchParams(params).toString();
  }

  try {
    const response = await fetch(url.toString());
    if (!response.ok) {
      throw new Error(`HTTP error: ${response.status}`);
    }
    return await response.json();
  } catch (error) {
    console.error(`Failed to fetch from API: ${url.toString()}`, error);
    throw error;
  }
}

export async function getProducts(params: FetchParams = {}) {
  return fetchFromApi("products", params);
}


try {
const response = await fetch(
`https://panda-market-api.vercel.app/products?${query}`
Copy link
Collaborator

Choose a reason for hiding this comment

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

https://panda-market-api.vercel.app/처럼 계속 사용되는 도메인은 보통 상수화 시켜둡니다!

import { Inter } from 'next/font/google'
import styles from '@/styles/Home.module.css'

const inter = Inter({ subsets: ['latin'] })
Copy link
Collaborator

Choose a reason for hiding this comment

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

font 들도 styles라는 폴더안에서 따로 관리해주고 css파일을 import 하는 방식을 사용해주는 것을 추천해요!

rel="noopener noreferrer"
>
By{' '}
<Image
Copy link
Collaborator

Choose a reason for hiding this comment

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

next/image 사용하신 것 너무 좋습니다!


try {
const response = await fetch(
`https://panda-market-api.vercel.app/products/${productId}`
Copy link
Collaborator

Choose a reason for hiding this comment

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

https://panda-market-api.vercel.app/ 요렇게 반복되는 도메인은 BASE_URL 같이 상수화 시켜두시고 사용하시면 오타날 위험도 줄고, 사용하기에도 더 편하겠죠!?

const BASE_URL = https://panda-market-api.vercel.app/

rel="noopener noreferrer"
aria-label="판다마켓 페이스북"
>
<img src={facebookLogo} alt="페이스북" width="20" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Next.js에서는 웬만해서 로 next/image 사용해주시는 것이 좋고
svg인 경우에는 컴포넌트인 것 처럼 만들어서 사용하실 수 있습니다 :)

예를 들면 요런식으로.. https://velog.io/@gkfla668/NextJS-%EC%97%90%EC%84%9C-SVG-%ED%8C%8C%EC%9D%BC-%EC%82%AC%EC%9A%A9%ED%95%98%EB%8A%94-%EB%B2%95-feat.svgr-%EC%9B%B9%ED%8C%A9-%EC%84%A4%EC%A0%95%ED%95%98%EA%B8%B0

{isDropdownVisible && (
<DropdownMenuContainer>
<DropdownItem
onClick={() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

onClick함수도 함수로 분리해서 공통적인 부분을 뺄 수 있을 것 같은데요!?

setIsVisible(false);
}, minLoadTime);

return () => clearTimeout(timer);
Copy link
Collaborator

Choose a reason for hiding this comment

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

clean up 함수 좋습니다~!!

{tags.length > 0 && (
<TagButtonsSection>
{tags.map((tag) => (
<Tag key={`tag-${tag}`}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

좋아요! 키 값은 항상 유니크해야 한다는 점만 잊지마세요 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. 미완성🫠 죄송합니다..
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants