-
Notifications
You must be signed in to change notification settings - Fork 3
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
KDT0_ParkJiSong 인터파크 메인 페이지 클론코 #58
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for glistening-salamander-d7cd38 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
많은 swiper 모두 구현하신게 대단한것같습니다! 그리고 이미지파일이 많은데 폴더링이랑 해상도 관리가 잘되어있어 보기 편했습니다! |
기존 페이지와 완벽에 가깝게 구현하신 거 같아서 대단해요🫢 다만 adBanner가 슬라이드 이미지 뒤 쪽으로 숨는 현상이 있습니다. position 값과 z-index값을 이용해서 맨 앞쪽으로 보이게 한다면 더 나은 ui가 될 것 같아요! |
swiper를 많이 사용하셨는데 네이밍만 보더라도 무슨 기능인지 바로 이해할수 있었습니다 BEM규칙을 알맞게 사용하신거같아요 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다!
js/script.js
Outdated
navigation:{ | ||
prevEl:'.swiper-prev', | ||
nextEl:'.swiper-next' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아래 navigation들도 다 같은 class name을 쓰다 보니, 이전 다음 버튼 눌렀을 때 스와이퍼들이 통째로 같이 움직이고 있어요...!
<!--css--> | ||
<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/[email protected]/reset.min.css"> | ||
<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/swiper@10/swiper-bundle.min.css"/> | ||
<link rel="stylesheet" href="./css/style.css"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여러 path에서 접근해도 파일을 정상적으로 불러오게 하려면 절대 경로를 사용하시는 게 좋아 보입니다.
index.html
Outdated
<div class="header__top"> | ||
<div class="header__top--left"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BEM 적용 시도하신 건 좋아 보입니다!
.header
부모 아래에.header__top
이 있지 않네요..header__top--left
는.header__top
의 상태를 변경할 때 쓰는 class name입니다.
위 두 가지만 추가로 참고하시면 좋을 것 같습니다!
index.html
Outdated
</div> | ||
</div> | ||
<div class="header__top--right"> | ||
<div class="myMenu">로그인</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class name에선 보통 dash(-)를 구분자로 사용합니다!
myMenu
-> my-menu
index.html
Outdated
<button class="swiper-prev">이전</button> | ||
<button class="swiper-next">다음</button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
button
이 예상 가능하게 동작하도록, button
을 사용하실 땐 가급적 type
attribute까지 작성하시는 걸 권장드립니다.
css/style.css
Outdated
width: 18px; height: 28px; | ||
background: url('../img/slideArrow.png') no-repeat 50%; | ||
} | ||
.swiper-button-disabled{display: none !important;} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!important
는 남용하지 않는 것을 권장드립니다.
이 상황에서는 !important
가 없어도 버튼이 잘 숨겨집니다.
.header__bottom .header__mainNav ul{ | ||
display: flex; | ||
justify-content: start; align-items: center; | ||
} | ||
.header__bottom .header__mainNav ul li{ | ||
display: flex; | ||
align-items: center; | ||
cursor: pointer; | ||
} | ||
.header__bottom .header__mainNav ul li+li{ | ||
margin-left: 22px; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
다양한 selector를 많이 활용하셨네요!
.header__bottom .header__mainNav ul {
display: flex;
justify-content: start;
align-items: center;
li {
display: flex;
align-items: center;
cursor: pointer;
+ li {
margin-left: 22px;
}
}
}
scss까지 활용하시면 이렇게 중복을 많이 줄일 수 있습니다.
css/style.css
Outdated
margin: 0 13px; | ||
} | ||
|
||
/*card__more*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
section을 구분하기 위해 사용된 주석 같은데, 작성된 class name과 대동소이한 내용들이라...조금 더 알아볼 수 있는 내용이 작성되는 게 아니라면 삭제되는 것도 가독성에 좋아 보입니다.
.ticket__swiper .card__img__text >div{ | ||
overflow: hidden; | ||
text-overflow: ellipsis; | ||
white-space: nowrap; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.hidden
처럼 이런 속성도 .text-ellipsis
로 묶을 수 있지 않았을까요?
.footer__policyList >li:hover .footer__policy--sub{ | ||
display: block; | ||
} | ||
.footer__policy--sub{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기 마우스가 안 올려지는 사소한 문제가 있습니다...
인터파크 메인 페이지 클론코딩
기간
2023-07-27~2023-07-28
프로젝트 내용
레퍼런스 : https://www.interpark.com/
데모 : https://glistening-salamander-d7cd38.netlify.app/
기능구현
아쉬운점