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

feat: 메인 페이지 #156

Closed
wants to merge 20 commits into from
Closed

Conversation

2taesung
Copy link
Collaborator

@2taesung 2taesung commented Oct 9, 2023

📖 작업 배경


🛠️ 구현 내용

  • 글로벌 헤더
  • 비동기 통신(ssr)
  • 리스트 컴포넌트 핸들링
  • 이미지 렌더링
  • 페이지네이션

💡 참고사항

  • 매우 비효율적이지만 공부 목적으로 server pagination 구현
  • styling template 사용

🖼️ 스크린샷

image

Comment on lines +13 to +16
"postcss": "8.4.29",
"react": "18.2.0",
"react-dom": "18.2.0",
"tailwindcss": "3.3.3"
Copy link
Collaborator

Choose a reason for hiding this comment

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

postcss랑 tailwindcss는 devDependencies에 위치하는게 좋을 것 같아요~

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

postcss랑 tailwindcss는 devDependencies에 위치하는게 좋을 것 같아요~

맞습니다 감사합니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

음 이 부분은 생각해보니 next를 설치하면서 같이 설치했던 패키지입니다!
말씀해주신대로 이것저것 따지면 옮길 수 있는 여지가 있는데 좀 검색해보니 ssr의 경우 문제될 부분들이 있다라고 하네요!
앞에서 언급드렸다시피 docs에서 next를 따라 설치하면서 잡아준 위치이다보니 기존 위치를 유지시키는게 좋을 것 같습니다!

언급해주셔서 제대로 공부하게 됐네요!

import Banner from '@/components/Banner';
import MainSection from '@/components/MainSection';

const PAGE_PER_NUM = 5;
Copy link
Collaborator

Choose a reason for hiding this comment

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

NUM_PER_PAGE가 조금 더 적절할 것 같아요~

image image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

NUM_PER_PAGE가 조금 더 적절할 것 같아요~

image image

오 이렇게 확인하는 방법이 있겠군요 감사합니다!


const PAGE_PER_NUM = 5;

async function getData({ page }: { page: number }) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

getData보다는 어떤 데이터를 가져오는지 의도를 드러내주면 어떨까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

getData보다는 어떤 데이터를 가져오는지 의도를 드러내주면 어떨까요?

맞는 의견입니다!
아무래도 확장성을 고려하지 않고 해당 페이지(서비스)에서 데이터가 해당 부분 하나라서 신경 못 썼던 것 같습니다!

const res = await fetch('https://api.realworld.io/api/articles');
const { articles } = await res.json();
const pageList = Array.from(
{ length: articles.length / PAGE_PER_NUM },
Copy link
Collaborator

Choose a reason for hiding this comment

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

articles.length가 21이면, 페이지당 5개의 article을 보여주고 있어서 1~5 페이지가 필요할 것 같은데요.
1: 1 ~ 5
2: 6 ~ 10
3: 11 ~ 15
4: 16 ~ 20
5: 21

length값은 4.2가 되어서 페이지가 총 4개만 존재하는 문제가 있을 것 같아요.

Copy link
Collaborator

Choose a reason for hiding this comment

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

articles.length로는 전체 articles의 개수를 알 수가 없을 것 같은데 GET /articles의 response중에 articlesCount 값을 사용하면 어떨까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

articles.length가 21이면, 페이지당 5개의 article을 보여주고 있어서 1~5 페이지가 필요할 것 같은데요. 1: 1 ~ 5 2: 6 ~ 10 3: 11 ~ 15 4: 16 ~ 20 5: 21

length값은 4.2가 되어서 페이지가 총 4개만 존재하는 문제가 있을 것 같아요.

네 맞습니다. 버림 처리 되는걸 놓쳤네요!
감사합니다 해당 로직을 추가해야겠습니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

articles.length로는 전체 articles의 개수를 알 수가 없을 것 같은데 GET /articles의 response중에 articlesCount 값을 사용하면 어떨까요?

articles.length로 전체 개수를 왜 알 수 없을까요 ??? articlesCount라는 값을 이용하면 훨씬 좋을 것 같긴한데 문제는 없을 것 같아서요!!

Copy link
Collaborator

Choose a reason for hiding this comment

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

GET /articles api는 offset, limit 값을 받도록 페이지네이션 처리가 되어있는데요.
따라서 해당 api를 호출하면 전체 데이터를 불러올 수가 없고 일부 아티클만 불러오기 때문에 전체 아티클의 개수는 확인할 수가 없어요. 실제로 확인해보면 해당 API를 호출한 결과를 담고 있는 articles 배열은 원소의 개수가 10개만 담겨있게 되어요. (Swagger에서 확인했을때는 limit값을 주지 않으면 default로 20이 들어간다고 하는데 실제로는 10개를 불러오는 것 같네요)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아.... 그렇군요 ... 제가 스웨거를 못 찾아서 없는줄 알았네요 ㅠ;

Comment on lines 13 to 16
const articleList = articles.splice(
(page - 1) * PAGE_PER_NUM,
page * PAGE_PER_NUM,
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

splice 메소드의 return 값은 제거한 요소로 알고 있는데요!
3page의 내용을 보고 싶은 경우에는 아래와 같이 될 것 같은데 그러면 10번째 인덱스부터 제거된 15개의 원소가 articleList에 담기지 않을까요?

  // page = 3, PAGE_PER_NUM = 5
  const articleList = articles.splice(
    (page - 1) * PAGE_PER_NUM, // 10
    page * PAGE_PER_NUM, // 15
  );

Copy link
Collaborator

Choose a reason for hiding this comment

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

데이터 전체를 가져와서(실제로 지금 전체를 가져오고 있지는 않아요) 페이지에 맞게 자르기 보다는 API에 pagination이 구현되어 있기 때문에 query parameter에 offset, limit값을 넣어줘도 원하는 데이터를 서버에서 바로 가져오면 어떨까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

데이터 전체를 가져와서(실제로 지금 전체를 가져오고 있지는 않아요) 페이지에 맞게 자르기 보다는 API에 pagination이 구현되어 있기 때문에 query parameter에 offset, limit값을 넣어줘도 원하는 데이터를 서버에서 바로 가져오면 어떨까요?

좋은 의견 감사합니다!
swagger를 못찾아 제대로 된 api를 확인 못하고 코드 작성해서 이런 삽질을 하게 됐네요 감사합니다!

);

if (!res.ok) {
throw new Error('Failed to fetch data');
Copy link
Collaborator

Choose a reason for hiding this comment

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

어떤 데이터를 fetch하는지 에러 메시지에 드러내줘도 좋을 것 같아요~

Comment on lines 23 to 27
<Suspense fallback={<Loading />}>
{articleList.map((article: Article) => {
return <Item key={article.slug} article={article} />;
})}
</Suspense>
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기서의 Suspense가 동작하나요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

실제로 동작하지 않습니다 ㅠ
next app router docs에서 알려주는대로 했는데 어떤 문제인지 파악을 못했어요 ㅠ

Copy link
Collaborator

Choose a reason for hiding this comment

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

docs에 보면<Suspense> works by wrapping a component that performs an asynchronous action (e.g. fetch data)라고 나와있는데요. 현재 Suspense로 래핑한 Item이라는 컴포넌트 내부에서 asynchronous action이 존재하지 않아서 그런 것 같아요~

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

docs에 보면<Suspense> works by wrapping a component that performs an asynchronous action (e.g. fetch data)라고 나와있는데요. 현재 Suspense로 래핑한 Item이라는 컴포넌트 내부에서 asynchronous action이 존재하지 않아서 그런 것 같아요~

그렇네요 제가 놓쳤던 부분입니다!
감사합니다!!!!!!

<div className="col-md-9">
<FeedToggle />
<Suspense fallback={<Loading />}>
{articleList.map((article: Article) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

article 타입은 TS에서 자동으로 추론이 될 것 같은데 컨벤션으로 쓰시는 걸수도 있겠네요~

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

article 타입은 TS에서 자동으로 추론이 될 것 같은데 컨벤션으로 쓰시는 걸수도 있겠네요~

추론이 돼서 안들어가도 되는데 코딩하다가 들어간걸 빼지 않았던 것 같습니다!!

Comment on lines 5 to 17
<ul className="pagination">
{pageList.map(num => {
return (
<Link
key={num}
className="page-item active"
href={`/home?page=${num}`}
>
{num}
</Link>
);
})}
</ul>
Copy link
Collaborator

Choose a reason for hiding this comment

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

이렇게 되면 ul 태그 아래에 li 태그가 없는 것 같아요.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이렇게 되면 ul 태그 아래에 li 태그가 없는 것 같아요.

감사합니다. 빠진 부분 추가해야겠습니다!

export default function Pagination({ pageList }: { pageList: Array<number> }) {
return (
<ul className="pagination">
{pageList.map(num => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

page라고 해주면 더 명확하게 느껴지지 않을까요?

Suggested change
{pageList.map(num => {
{pageList.map(page => {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

page라고 해주면 더 명확하게 느껴지지 않을까요?

의견 감사합니다!
이건 pageList -> page가 자연스럽긴 한데
실제로 num이 적절한 item의 변수가 될 것 같아서 ....
pageList를 더 적절한 변수명으로 바꿔줘야 좋을 것 같다는 생각이 드네요!!

Copy link
Collaborator

Choose a reason for hiding this comment

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

네~ 저도 애초에 pageList라는 변수명이 애매해서 생긴 문제라고 생각해요~

@InSeong-So InSeong-So closed this May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants