-
Notifications
You must be signed in to change notification settings - Fork 3
KDT0_JangHojin #62
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
base: main
Are you sure you want to change the base?
KDT0_JangHojin #62
Conversation
✅ Deploy Preview for mellifluous-gnome-003496 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for willowy-truffle-a69129 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
content 안의 div들은 section으로 묶어도 좋을 것 같아요! |
|
기존 페이지에 있는 헤더 호버 스타일이라던지, 드롭다운 스크롤 이벤트 등 같이 구현되었으면 더 좋았을 거 같아요! |
|
다양한태그들을 알맞은 곳에 시맨틱태그로 정말 잘 사용하신거같아요 |
marshallku
left a comment
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.
고생하셨습니다!
|
|
||
| </body> | ||
|
|
||
| </html> No newline at end of file |
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.
Prettier 설정이 필요해 보이네요!
| * { | ||
| outline: 0; | ||
| } | ||
|
|
||
| p { | ||
| display: block; | ||
| margin-block-start: 1em; | ||
| margin-block-end: 1em; | ||
| margin-inline-start: 0px; | ||
| margin-inline-end: 0px; | ||
| } | ||
|
|
||
| html, body, div, span, object, iframe, h1, h2, h3, h4, h5, h6, p, blockquote, pre, abbr, address, cite, code, del, dfn, em, img, ins, kbd, q, samp, small, strong, sub, sup, var, b, i, dl, dt, dd, ol, ul, li, fieldset, form, label, legend, table, caption, tbody, tfoot, thead, tr, th, td, article, aside, canvas, details, figcaption, figure, footer, header, hgroup, menu, nav, section, summary, time, mark, audio, video { | ||
| margin: 0; | ||
| padding: 0; | ||
| border: 0; | ||
| box-sizing: border-box; | ||
| } |
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가 섞여있으니 읽기 헷갈려지는 부분이 좀 있는 것 같네요.
목적에 따라 파일을 분리해보는 것도 좋을 것 같습니다!
css/main.css
Outdated
| left: 0; | ||
| right: 0; | ||
| height: 72px; | ||
| transition: .4s; |
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.
가급적이면 transition은 필요한 속성에만 주시는 게 좋습니다!
지금은 transition될 일이 아예 없는 것 같아서, 삭제하시는 게 좋아 보이네요.
css/main.css
Outdated
| header section .btn_srch { | ||
| position: absolute; | ||
| border: none; | ||
| text-indent: -9999px; |
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/main.css
Outdated
| border: none; | ||
| text-indent: -9999px; | ||
| top: 22px; | ||
| right: 396px; |
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.
화면 변화에 유동적으로 대응하기 힘들어 보입니다.
header > section을 flex box로 만들면, position 조정 없이 현재 레이아웃과 같은 모습을 만드실 수 있습니다!
index.html
Outdated
| </head> | ||
|
|
||
| <body class="pc"> | ||
| <link rel="stylesheet" href="./css/main.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.
<body> 아래에 배치시키신 이유가 혹시 있나요?
stylesheet이 body-ok type이긴 하지만, <head> 내에 위치시키는 게 best practice입니다.
추가로, path 영향 없이 파일을 불러올 수 있도록, 절대 경로 사용을 습관화해보시면 좋을 것 같습니다.
index.html
Outdated
| <h1> | ||
| <a class="" href="" title="여기어때"></a> | ||
| </h1> | ||
| <button type="button" class="btn_srch srch_open " style="right: 396px;"></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.
성능과 재사용성을 위해, 가급적 스타일은 css 파일에 작성하시길 권장드립니다.
index.html
Outdated
| <li><a href="">내주변</a></li> | ||
| <li><a href="">예약내역</a></li> | ||
| <li class="over"> | ||
| <button type="button"><span>더보기</span></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.
type까지 명시적으로 작성해주신 거 좋네요!
index.html
Outdated
| <div class="recommend"> | ||
| <h2>여기어때 소식</h2> | ||
| <ul> | ||
| <li><a href="" target="_blank"><img src="//image.goodchoice.kr/images/web_v3/b2b_banner.png" |
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.
Tab nabbing 공격 등을 바지하기 위해, target="_blank"로 새 탭에서 창을 열 땐 rel="noopener noreferrer"까지 추가해주는 게 좋습니다.
index.html
Outdated
|
|
||
| </div> | ||
|
|
||
| <button class="btn_go_top" onclick="moveTop();" style="display: none;">상단으로</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.
script가 지워진 것 같은데 확인 한 번 부탁드릴게요!
No description provided.