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

KDT5_LeeJiHun #75

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

KDT5_LeeJiHun #75

wants to merge 1 commit into from

Conversation

luckfeat
Copy link

@luckfeat luckfeat commented Apr 9, 2023

공식 사이트 링크

https://www.playstation.com/en-us/

클론 사이트 링크

https://ui-playstation.netlify.app/

Copy link
Member

@GyoHeon GyoHeon left a comment

Choose a reason for hiding this comment

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

총평

상당히 짜임새있는 프로젝트입니다.
특히 HTML의 구조와 짜임새가 아주 좋습니다.
다만, div의 사용이 너무 많아 아쉽습니다.
다음엔 시맨틱한 태그의 사용이 늘어났으면 합니다.

자세한 내용들은 코멘트에 달아두었습니다.

수고하셨습니다!

}

* {
font-family: 'sst', cursive, sans-serif;
Copy link
Member

Choose a reason for hiding this comment

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

예비 폰트를 지정해 준 것이 좋습니다.

<body>
<div class="sony-bar">
<a href="javascript:void(0)">
<span class="sony-logo"></span>
Copy link
Member

Choose a reason for hiding this comment

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

h1 태그는 페이지의 제목을 나타내는 요소로, 페이지당 1개의 h1 요소가 적극 추천됩니다.

이미지를 h1 태그로 감싸도 alt가 그 역할을 대신할 수 있기 때문에, h1 태그의 사용을 권장합니다.

참고사항 : https://developer.mozilla.org/ko/docs/Web/HTML/Element/Heading_Elements

<h1>은 페이지에서 한 번만 사용되는 것이 일반적입니다. (웹페이지 자체의 제목 등)반복되는 제목은 <h2> or <h3>... 나 <strong>등을 이용하여 나타내는 것이 좋습니다.

페이지 당 하나의 <h1>만 사용하세요. 여러 개를 써도 오류는 나지 않겠지만, 단일 <h1>이 모범 사례로 꼽힙니다. 논리적으로 생각했을 때도, <h1>은 가장 중요한 제목이므로 전체 페이지의 목적을 설명해야 할 것입니다. 두 개의 제목을 가진 책이나, 여러 개의 이름을 가진 영화는 볼 수 없죠! 또한 스크린 리더 사용자와 [SEO](https://developer.mozilla.org/ko/docs/Glossary/SEO)에도 더 적합합니다.

</div>
</header>
<!-- Main -->
<section>
Copy link
Member

Choose a reason for hiding this comment

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

main이라면 main 태그를 사용하는게 더 시맨틱합니다.

Comment on lines +379 to +385
<div class="button">
<a href="javascript:void(0)" class="blue">
<div class="button__inner">
<span>Learn More</span>
</div>
</a>
</div>
Copy link
Member

Choose a reason for hiding this comment

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

  1. button 이라면 button 태그를 사용하는게 더 시맨틱하겠죠?
  2. div > a > div > span 의 구조가 전부 같은 범위를 감싸고 있습니다. 굳이 DOM depth를 늘리지 않고 간단하게 만들 수 있을 것 같습니다.

Comment on lines +5 to +15
window.addEventListener('scroll', () => {
if (!navIsScrolled && scrollY > 37) {
navIsScrolled = true;
sharedNav.classList.add('scrolled');
gameWrapper.classList.add('scrolled');
} else if (navIsScrolled && scrollY < 37) {
navIsScrolled = false;
sharedNav.classList.remove('scrolled');
gameWrapper.classList.remove('scrolled');
}
});
Copy link
Member

Choose a reason for hiding this comment

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

position: sticky; 로도 같은 스타일을 재연할 수 있습니다.
css로 처리가능한 로직들은 css로 처리하는게 좋습니다.

참고사항 : https://developer.mozilla.org/en-US/docs/Web/CSS/position

Comment on lines +45 to +48
<header>
<div class="shared-nav-container">
<div class="shared-nav">
<nav class="shared-nav--menu-closed">
Copy link
Member

Choose a reason for hiding this comment

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

header > div > div > nav의 범위가 모두 같습니다.

따라서, div를 없애고 스타일링을 통해서 DOM depth를 줄일 수 있습니다.

cursor: pointer;
}

.shared-nav__primary-item button span {
Copy link
Member

Choose a reason for hiding this comment

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

선택자에 클래스와 태그를 적절히 사용하는 것이 아주 좋습니다.

Comment on lines +235 to +243
.game-wrapper
.swiper-container
.banner
.banner__content-wrapper
.inner-div
.banner-logo {
opacity: 0;
width: 422.953px;
}
Copy link
Member

Choose a reason for hiding this comment

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

클래스를 시맨틱하게 지었으므로, 이렇게 모든 선택자를 사용하지 않아도 됩니다.

시맨틱한 클래스 네임의 이점을 살리면 선택자도 간결해 질 수 있습니다.

Copy link
Member

Choose a reason for hiding this comment

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

또한, 픽셀은 정수 단위로 나타나기 때문에 소수점은 큰 의미가 없습니다.

.inner-div
.banner-logo
.banner-logo-inner {
width: calc((37.16px * 4) + (3 * 2.5rem));
Copy link
Member

Choose a reason for hiding this comment

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

calc의 사용은 좋지만, 굳이 필요하지 않은 곳에서의 사용은 지양합시다.

해당 부분은 굳이 calc를 사용하지 않아도 괜찮을 것 같습니다.

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.

2 participants