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

KDT0_ParkNAYoung LG U+ 홈페이지 클론코딩 (기존 PR closed 후) #75

Open
wants to merge 47 commits into
base: main
Choose a base branch
from

Conversation

im-na0
Copy link

@im-na0 im-na0 commented Jul 31, 2023

LG U+ 홈페이지 클론코딩

기간

2023-07-24~2023-07-28


프로젝트 내용

레퍼런스 : https://www.lguplus.com/

데모 : https://glowing-chebakia-807feb.netlify.app/


기능구현

  • 시멘틱 태그를 활용했습니다
  • flex를 이용했습니다
  • JS로 애니메이션을 추가했습니다
  • Swiper를 이용해 슬라이드를 기능을 추가했습니다

아쉬운점

  • 반응형을 고려하지 못했습니다
  • class를 너무 과도하게 사용했습니다
  • scrollTop 기능을 구현하지 못했습니다

개선한 사항

  • 좀 더 시맨틱한 구조로 변경했습니다
  • SASS 사용하여 CSS를 작성했습니다
  • 오늘하루그만보기 기능을 구현했습니다

Copy link
Member

@marshallku marshallku left a comment

Choose a reason for hiding this comment

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

고생하셨습니다!

.DS_Store Outdated
Copy link
Member

Choose a reason for hiding this comment

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

이런 OS가 생성한 파일은 커밋되지 않도록 .gitignore에 추가해주시는 게 좋습니다!

css/common.css Outdated
Comment on lines 157 to 160
[type='button']:not(:disabled),
[type='reset']:not(:disabled),
[type='submit']:not(:disabled),
button:not(:disabled) {
Copy link
Member

Choose a reason for hiding this comment

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

buton:not(:disabled)만 있어도 되지 않을까요?

js/main.js Outdated
*/
const swiper = new Swiper('.visual .swiper-container', {
autoplay: {
delay: 5000, // 5초마다 슬라이드 바뀜
Copy link
Member

Choose a reason for hiding this comment

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

const SECONDS_TO_MS = 1000처럼 상수를 하나 선언해두시고, delay: 5 * SECONDS_TO_MS처럼 작성하시면, 주석 없이도 쉽게 알아볼 수 있는 코드를 작성하실 수 있습니다.

js/main.js Outdated
Comment on lines 9 to 11
if (!bnEl || !clsBtn || !onTodayCls) {
return // 요소가 존재하지 않으면 함수 종료
}
Copy link
Member

Choose a reason for hiding this comment

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

runtime error 안 생기게 early exit 해주신 거 좋습니다!

js/main.js Outdated
let isPause = false

tab.addEventListener('click', function () {
isPause = !isPause
Copy link
Member

Choose a reason for hiding this comment

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

현재 상태에 따라 로직을 처리하는 거니, 각종 로직 처리가 끝난 뒤 isPause를 뒤집으시면 조금 더 가독성이 좋아지지 않을까요?

css/footer.css Outdated
Comment on lines 241 to 267
.footer-sns .facebook {
background-image: url('../asset/footer-sns-facebook.svg');
}

.footer-sns .youtube {
background-image: url('../asset/footer-sns-youtube.svg');
}

.footer-sns .twitter {
background-image: url('../asset/footer-sns-twitter.svg');
}

.footer-sns .lgblog {
background-image: url('../asset/footer-sns-lgblog.svg');
}

.footer-sns .insta {
background-image: url('../asset/footer-sns-instagram.svg');
}

.footer-sns .kakaostory {
background-image: url('../asset/footer-sns-kakao.svg');
}

.footer-sns .naverblog {
background-image: url('../asset/footer-sns-naverblog.svg');
}
Copy link
Member

Choose a reason for hiding this comment

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

$social-media: facebook, youtube, twitter, lgblog, insta, kakaostory, naverblog;

.footer-sns {
  @each $item in $social-media {
    .#{$item} {
      background-image: url(/asset/footer-sns-#{$item}.svg);
    }
  }
}

sass를 사용하시면 이렇게 깔끔하게 정리할 수 있으니, 참고해보시면 좋을 것 같습니다!

css/footer.css Outdated
Comment on lines 118 to 119
height: 40px;
min-height: 40px;
Copy link
Member

Choose a reason for hiding this comment

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

height: 40px만 남아도 되지 않나요?

css/header.css Outdated
Comment on lines 30 to 37
.c-chkbox input[type='checkbox'] {
position: absolute;
left: 0px;
outline: none;
border: 0;
width: 22px;
height: 22px;
display: none;
Copy link
Member

Choose a reason for hiding this comment

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

display: none으로 숨기시면 다른 값들은 없어도 되지 않을까요?

css/main.css Outdated
Comment on lines 302 to 316
.item-container .item-box:first-child {
background-image: url('../asset/20230720-025128-230-c2sLz2P5.jpg');
}

.item-container .item-box:nth-child(2) {
background-image: url('../asset/20230717-010142-134-3vpueBDv.jpg');
}

.item-container .item-box:nth-child(3) {
background-image: url('../asset/20230711-035937-777-j0u8IyaO.jpg');
}

.item-container .item-box:last-child {
background-image: url('../asset/20230628-043246-125-KnHNERTf.jpg');
}
Copy link
Member

Choose a reason for hiding this comment

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

background-image를 사용하면 alt 속성 등의 활용을 강제하기 쉽지 않으니, 이런 상황에선 <img>를 사용하는 게 좋지 않을까 합니다.
추가로, .item-box가 추가될 때 문제가 생기지 않으려면 :last-child 대신 :nth-child(4)를 쓰는 것도 좋아 보입니다.

css/main.css Outdated
.item-box.fade-in {
opacity: 0;
transform: translateY(50px);
transition: all 0.5s ease-in-out;
Copy link
Member

Choose a reason for hiding this comment

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

가급적 transition은 실제로 사용할 속성에만 주시는 게 좋습니다.

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