-
Notifications
You must be signed in to change notification settings - Fork 39
[박재현] Sprint1 #18
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
[박재현] Sprint1 #18
The head ref may contain hidden characters: "Basic-\uBC15\uC7AC\uD604-sprint1"
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.
재현님 첫번째 PR 제출 고생하셨습니다!
리뷰 드린 코멘트는 모두 필수 반영사항은 아니고 코드에 정답은 없으니
보고 마음에 드시는 방향으로 수정하시면 좋겠습니다!
다음 주차도 화이팅입니다~
- commit 단위를 나눠 진행하신 것은 좋습니다. 다만 어떤 작업을 수행하신 건지 알 수 있는 commit 메세지로 적어주시면 좋겠습니다!
- 다음에는 배포주소도 같이 올려주시면 확인할때 더 좋을 것 같습니다~
- 다음 PR 올리실때, target branch를 main이 아니라 재현님 할당 브랜치인 Basic-박재현으로 설정해주세요~
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.
❗️ 수정요청
재현님 해당 파일은 mac에서 작업하실 때 생성되는 파일입니다.
해당 폴더에 대한 정보를 가지고 있는 파일로, 이러한 파일들은 git repo에 올라갈 이유가 없습니다~
git repo에는 공유할필요가 있는 파일들만 올라가면 되기 때문에 이러한 파일들은 git에 올라가지 않도록 관리하는 것이 좋습니다.
관련한 블로그 글이 있으니 읽어보시고, 제거하시는 것을 추천드릴께요~
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.
💬 여담
figma에서 그때 그때 필요한 이미지만 추출하시는 것을 추천드려요~
아마 같이 사용하는 디자인 파일이라 export 탭에서 export 처리된 친구들을 다 추출하게 되시면
불필요한 친구들도 같이 딸려올 가능성이 큽니다!
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.
| @@ -0,0 +1,116 @@ | |||
| <!DOCTYPE html> | |||
| <html lang="kr"> | |||
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.
❗️ 수정요청
html lang 속성은 스크린리더, 검색 엔진등에게 해당 사이트가 어떤 언어 기반으로 작성되었는지 설명하는 속성입니다.
해당 속성에는 이미 특정 언어들을 의미하는 예약어가 들어가야 합니다. 예를들어 en은 english 즉 영어를 의미합니다.
kr은 신기하게도 Kanuri 어를 의미합니다. 한국어는 ko를 써야합니다~
https://developer.mozilla.org/ko/docs/Web/HTML/Global_attributes/lang
(해당 페이지 sidebar LockUp 에서 kr과 ko를 검색해보세요~)
https://r12a.github.io/app-subtags/
| <html lang="kr"> | |
| <html lang="ko"> |
| <header> | ||
| <nav class="nav-bar"> | ||
| <a href="/" class="nav-logo"> | ||
| <img src="assets/logos/nav-logo.png" alt="img-nav-logo" /> |
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.
💊 제안
img 태그의 alt 속성은 alternative 라는 의미로, 이미지 파일을 다운로드하는 것에 실패해서 이미지 파일을 보여줄 수 없을 때
어떤 이미지인지 파악할 수 있게 대신 제공되거나, 스크린리더로 읽혀지는 문자를 의미합니다.
따라서 가능한 이미지를 설명할 수 있는 대체 텍스트를 입력해주세요~
| <img src="assets/logos/nav-logo.png" alt="img-nav-logo" /> | ||
| </a> | ||
| <div class="nav-spacer"></div> | ||
| <a href="/login" class="nav-login-button"> 로그인 </a> |
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 로 스타일링 할때 의도치 않은 동작을 할 수도 있고, 의도가 있는 공백인지 아닌지 이해하기 어렵기 때문입니당.
| <a href="/login" class="nav-login-button"> 로그인 </a> | |
| <a href="/login.html" class="nav-login-button">로그인</a> |
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.
💬 여담
구현을 완료하신 후에는 작업하신 사항들이 잘 구현되엇는지, a tag, button tag들을 눌러보시면서 확인하시면 좋습니다~
| /> | ||
| <article class="main-content-text"> | ||
| <h2 class="category hot-item">Hot item</h2> | ||
| <h1 class="title hot-item"> |
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.
❗️ 수정요청
h1 태그는 heading 1이라는 의미의 태그입니다. 이러한 h1~h6까지의 태그들은 시멘틱한 작성에 중요한 역할을 합니다.
그중 h1 태그는 제목에 해당하는 친구입니다.
목차나 영화를 생각해보시면 제목은 늘 1개입니다. 그래서 h1태그는 한 페이지에 한번만 사용하는 것이 좋습니다.
다만 이는 접근성을 위한 권고사항일 뿐 필수사항은 아닙니다 그래도 권고니 지키는것이 더 좋습니다!
| <div class="footer-social"> | ||
| <a href="https://www.facebook.com" | ||
| ><i class="fa-brands fa-facebook"></i | ||
| ></a> | ||
| <a href="https://www.x.com"><i class="fa-brands fa-twitter"></i></a> | ||
| <a href="https://www.youtube.com" | ||
| ><i class="fa-brands fa-youtube"></i | ||
| ></a> | ||
| <a href="https://www.instagram.com" | ||
| ><i class="fa-brands fa-instagram"></i | ||
| ></a> | ||
| </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.
💊 제안
이 영역은 로고들의 모음이라고 할 수 있습니다. 관련된 요소들끼리 묶인 목록이니 의미록적인 마크업을 작성한다면 list 관련 태그로 작성하시는 것을 추천드립니다.
| <div class="footer-social"> | |
| <a href="https://www.facebook.com" | |
| ><i class="fa-brands fa-facebook"></i | |
| ></a> | |
| <a href="https://www.x.com"><i class="fa-brands fa-twitter"></i></a> | |
| <a href="https://www.youtube.com" | |
| ><i class="fa-brands fa-youtube"></i | |
| ></a> | |
| <a href="https://www.instagram.com" | |
| ><i class="fa-brands fa-instagram"></i | |
| ></a> | |
| </div> | |
| <ul class="footer-social"> | |
| <li> | |
| <a href="https://www.facebook.com"> | |
| <i class="fa-brands fa-facebook"></i> | |
| </a> | |
| </li> | |
| </ul> |
| a { | ||
| cursor: pointer; | ||
| text-decoration: none; | ||
| } |
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.
💊 제안
브라우저 마다 자체적으로 스타일을 가지고 있습니다.
위의 코드에서 a 태그의 text-decoration: none을 적용해주신건, 이미 text-decoration: underline 속성을 가지고 있기 때문이죠~
이런식으로 이미 가지고 있는 스타일을 없애 원하는 스타일을 적용하기 위한 작업이 필요합니다.
주로 이러한 역할을 하는 css 파일을 reset.css 혹은 normalize.css 라는 이름으로 만들어서 관리합니다.
재현님께서는 지금 필요한 태그의 스타일만 초기화하고 사용하셨지만 reset.css 같은 파일을 만드셔서
다른 태그들의 기본 스타일도 초기화하고 작업하시는 것이 예상치 못한 동작을 방지할 수 있어서 추천드려요!
혹시 어떤 내용들이 들어가는지 궁금하시다면 reset.css, normalize.css 를 구글에 검색해보시면 다양한 파일이 나오니
확인해보시고 마음에 드는 것을 참고하시면 좋을 것 같습니다.
| font-weight: 600; | ||
| } | ||
|
|
||
| /* 메인 컨텐츠 */ |
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 들을 모아두시고 주석으로 구분해주신점 좋습니다!


요구사항
기본
심화
주요 변경사항
스크린샷
멘토에게