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(land): migration landing page and create first section #257

Merged
merged 13 commits into from
Nov 26, 2024
Merged

Conversation

Jeong-Ag
Copy link
Contributor

Summary

#181

랜딩 페이지를 next.js로 마이그레이션 하고 메인 페이지의 첫 번째 뷰를 생성합니다.

Tasks

  • next.js 15로 upgrade
  • 레이아웃 설정
  • 메인 페이지 내 첫 번째 뷰 생성
  • 기본 폰트 및 디렉토리 설정

To Reviewer

next.js에 익숙하지 않아 실수가 많을 수 있어요 ........ 🥲 따끔한 충고 부탁드려요

Screenshot

스크린샷 2024-11-23 오후 3 25 07 스크린샷 2024-11-23 오후 3 25 10 스크린샷 2024-11-23 오후 7 28 48

@Jeong-Ag Jeong-Ag added ✨ Feature 새로운 기능 명세 및 개발 🎈Land land 프로젝트 관련 labels Nov 23, 2024
@Jeong-Ag Jeong-Ag requested review from gwansikk and SWARVY November 23, 2024 10:58
@Jeong-Ag Jeong-Ag self-assigned this Nov 23, 2024
Copy link

changeset-bot bot commented Nov 23, 2024

⚠️ No Changeset found

Latest commit: aed5c79

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

@SWARVY SWARVY left a comment

Choose a reason for hiding this comment

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

수고 많으셨어요 :) 아래 리뷰 검토 부탁드립니다

<p>© C-Lab. All rights reserved.</p>
</ul>
</div>
<a href={INFORMATION_URL.GITHUB} target="_blank">
Copy link
Contributor

Choose a reason for hiding this comment

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

Next에서는 a 태그 대신 next/link 컴포넌트를 사용하는 것을 추천드려요!
next/link 기본적으로 지원하는 기능이 더 많으니 한번 읽어보세요

target="_blank" 때문에 a 태그를 사용하신거라면, next/link에서도 동일하게 설정해줄 수 있어요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GitHub URL이 외부 링크라서 next/link의 이점이 적용되지 않을 것 같아 a 태그를 사용했어요. 이런 경우엔 a 태그가 더 직관적이지 않을까 했는데 일관성 있게 next/link를 사용하는 것이 더 좋을까요?

Copy link
Contributor

Choose a reason for hiding this comment

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

그게 더 좋을거같아요. 일관성을 가지는게 더 좋아보여요

Comment on lines 69 to 80
<Button
className="bg-clab-yellow border-clab-yellow text-clab-yellow rounded-full bg-opacity-30 px-4"
onClick={() =>
window.open(
INFORMATION_URL.MEMBERS,
'_blank',
'noopener noreferrer',
)
}
>
VISIT MEMBERS
</Button>
Copy link
Contributor

Choose a reason for hiding this comment

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

의도는 이해하지만 Button의 onClick으로 새 창을 여는 행위는 결국 a태그의 기능과 동일해지는 것 같은데, 어떻게 생각하시나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

요소의 의미를 명확하게 하기 위해 Button을 사용했는데 기능이 새 창을 여는 동작이니까 불필요하게 Button을 사용하는 것 보단 a 태그를 통해 이동시키는 것이 적합할 것 같네요! 수정하도록 하겠습니다 :)

@@ -0,0 +1,92 @@
import { useEffect, useRef, useState } from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

요거 next라서 'use client' 붙여야 작동할텐데, 혹시 개발환경에서 정상적으로 작동됐나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

엇 무사히 동작해서 넣었다가 없앴는데... use client 사용해서 명확하게 하는게 좋을 것 같네요
추후에 오류 발생할 수도 있으니 추가하겠습니다!

@@ -0,0 +1,35 @@
'use client';
Copy link
Contributor

Choose a reason for hiding this comment

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

기본적으로 next app router에서 page.tsx는 서버 컴포넌트를 권장하고있어요

해당 페이지에서는 useRouter를 사용하기 위해 라우팅에 사용되는 page.tsx를 클라이언트 컴포넌트로 사용하고있어요, 또 이는 router를 사용하여 페이지를 이동하기 위함인데 해당 이유로는 page.tsx를 클라이언트 컴포넌트로 사용해야할 당위성이 부족하다고 느껴져요

Button의 onClick으로 페이지를 이동시키는 것이 아니라 next/link를 사용하여 페이지를 이동하는 것 이었다면 해당 페이지는 클라이언트 컴포넌트일 필요가 없었을 거에요.

불필요한 클라이언트 컴포넌트의 사용은 불필요한 번들의 용량 증가를 의미해요. 따라서 해당 컴포넌트에서는 router를 사용하지 않고 link를 통해서 페이지를 이동하도록 변경하는게 좋을 것 같아요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

next/link를 사용하여 페이지를 이동시키면 클라이언트 컴포넌트가 필요하지 않겠네요. 성능면에서 생각하지 못했는데 알려주셔서 감사해요
useRouter 사용하지 않고 next/link를 사용하도록 수정하겠습니다!

direction: string;
}

export const TextSlider = ({ keywords, direction }: TextSliderProps) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

컴포넌트 생성이 현재 화살 함수 / 일반 함수 선언문 총 두가지 방식으로 생성되고 있어요.
해당 방법을 사용해야만 하는게 아니라면, 한가지 방법으로만 컴포넌트를 생성하는게 더 좋을 것 같아요 :)

Comment on lines 18 to 23
<h1
key={keyword}
className="text-clab-yellow-gray font-dung-geun-mo text-9xl font-bold"
>
{keyword}
</h1>
Copy link
Contributor

Choose a reason for hiding this comment

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

SEO적인 관점에서 h1 태그가 여러개 존재하는것은 권장되지 않는 패턴이에요
그 이유는 아래와 같아요 (지피티를 통해 요약된 내용이에요)

검색 엔진 최적화(SEO)

검색 엔진은 h1 태그를 페이지의 가장 중요한 제목으로 간주합니다.
여러 개의 h1 태그가 존재하면, 검색 엔진이 페이지의 주요 주제를 이해하는 데 혼란을 겪을 수 있습니다.
이는 검색 순위에 부정적인 영향을 미칠 가능성이 있습니다.

웹 접근성(Accessibility)

스크린 리더와 같은 보조 기술은 h1 태그를 기준으로 페이지 구조를 파악합니다.
여러 개의 h1 태그가 있으면 사용자 경험이 저하될 수 있습니다.

문서 구조와 의미론

HTML5에서 h1은 문서의 최상위 제목으로 사용됩니다.
문서 구조는 논리적이어야 하며, 하나의 h1을 중심으로 계층적으로 구성된 h2, h3 등이 있어야 합니다.

따라서 해당 부분은 다른 태그를 통해 나타내는 것이 좋을 것 같아요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

헉 좋은 내용 감사합니다!! 알려주신 내용을 보니 SEO와 접근성 측면에서 h1 태그가 부적합하다는 점이 잘 이해됐어요. 다른 태그 사용하도록 하겠습니다 ☺️

@Jeong-Ag Jeong-Ag requested a review from SWARVY November 24, 2024 07:30
@SWARVY
Copy link
Contributor

SWARVY commented Nov 24, 2024

LGTM. 수고하셨습니다 :)

@Jeong-Ag Jeong-Ag merged commit 2f9d0be into main Nov 26, 2024
4 checks passed
@gwansikk gwansikk deleted the feature/181 branch November 26, 2024 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Feature 새로운 기능 명세 및 개발 🎈Land land 프로젝트 관련
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants