-
Notifications
You must be signed in to change notification settings - Fork 0
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
과제 #37
base: main
Are you sure you want to change the base?
과제 #37
Conversation
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.
전체적으로 고생 많으셨습니다
아쉬운점은 코멘트 남겨드렸습니다
css파일 분기는 이해가 되지않네요 기준점을 잡아두고 미디어쿼리를 쓰셨으면 좋았을텐데요
<link rel="stylesheet" type="text/css" href="//db.onlinewebfonts.com/c/79e4dc6a81171b1523795cb6b2bc7b2f?family=Avenir+LT+Pro+55+Roman"/> | ||
<link rel="stylesheet" href="./css/common.css"> | ||
<link rel="stylesheet" href="./css/mobile.css"> | ||
<link rel="stylesheet" media="all and (min-width:770px)" href="./css/pc.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.
미디어쿼리를 쓰지 않으신 이유가 있나요?
<meta property="og:url" content="https://www.socar.kr/"> | ||
|
||
<link rel="icon" href="https://www.socar.kr/images/i-appicon.svg"> | ||
<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/[email protected]/reset.min.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.
안에 파일은 확인해보시고 사용하시는거맞나요?
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.
보통 reset.css를 안에 내용도 확인안하고 사용해서 css를 overwrite이슈가 생기는 케이스를 여럿 봤어서 말씀드렸습니다
<section class="main-visual scroll-spy"> | ||
<header> | ||
<a href="/"><img src="./images/logo-w.svg" alt="logo" id="logo"></a> | ||
<div class="menubar"> |
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.
nav태그를 쓰지 않으신 이유가 있나요?
|
||
<!-- DOWNLOAD BANNER --> | ||
<div class="download"> | ||
<a href="javascript:void(0)"> |
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.
href에 그냥 #같은걸로 넣어주는게 훨씬 보기 좋을 것 같아요
|
||
const spyEls = document.querySelectorAll('section.scroll-spy'); | ||
spyEls.forEach(function(spyEl){ | ||
new ScrollMagic |
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.
이렇게 쓰는게 맞는걸까요??
만약 이 기능이 해당 지점에 보이는걸 구현한거였다면, Intersection Observer를 알아보셔도 좋을 것 같아요
this.timer = null; | ||
this.counter(); | ||
}; | ||
numberCounter.prototype.counter = function() { |
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.
prototype 메서드를 쓰는 시도는 잘했지만,
numberCounter를 한번만 쓸거라면 차라리 클로저를 사용해보거나, 클래스를 사용해보는 방법도 생각해볼 수 있을 것 같아요
|
||
// members 영역 숫자 올라가는 효과 | ||
|
||
function numberCounter(target_frame, target_number) { |
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.
그리고 기본적으로 생성자 함수로 사용하실거라면 시맨틱상 맨 앞글자는 대문자가 원칙입니다
쏘카 페이지를 클론 코딩했습니다.
원본 페이지 : https://www.socar.kr/
과제 페이지 : https://genuine-palmier-371aa9.netlify.app/