[문혜란] sprint1 #27
[문혜란] sprint1 #27
Conversation
- reset.css CDN 적용하여 브라우저 기본 스타일 초기화 - h1 태그를 사용하여 로고 강조(SEO 및 웹 접근성 개선) - footer의 social 영역을 ul-li 구조로 변경 - img 태그의 alt 속성 수정 - 모바일 환경을 위한 로고 추가
There was a problem hiding this comment.
혜란님 첫번째 PR 제출 고생하셨습니다!
리뷰 드린 코멘트는 모두 필수 반영사항은 아니고 코드에 정답은 없으니
보고 마음에 드시는 방향으로 수정하시면 좋겠습니다!
다음 주차도 화이팅입니다~
- git commit 단위 쪼개시고 형식에 따라 commit message 작성하신 것 좋습니다!
- 시멘틱하게 태그 작성하시는 점 좋습니다.
- 배포 사이트에서
구경하러가기버튼이 디자인과 다른 것 같아요~ 확인 후 수정해보시면 좋겠습니다. 반응형은 아직 구현하지 못했습니다. 추후 스프린트 미션2 진행하면서 반영하도록 하겠습니다.: 추후 반응형 디자인이 제공되니 그때 작업하셔도 좋을 것 같아요~ 저는 목표가 없이 반응형 단위로 변경하는게 쉽진 않더라구요!
There was a problem hiding this comment.
❗️ 수정요청
해당 이미지 파일의 크기를 보면 588× 444로 되어 있습니다.
저희는 반응형 사이트를 만들고 있고, 이런 이미지들은 모바일에서도 사용할 것이기 때문에 추출하실때, 1배수가 아닌 2배수로 추출해주시는 것이 화질면에서 좋습니다.
아래처럼 figma export 탭에서 2x 로 설정하고 추출해주세요~
There was a problem hiding this comment.
💊 제안
한 레포안에서는 동일한 룰을 따르는 것이 좋습니다.
따라서 img_home_01.png와 같은 식으로 같은 네이밍 룰을 따르시는 것을 추천드려요~
| <h1> | ||
| <a href="/" class="logo"> | ||
| <img src="/assets/logo.png" alt="판다마켓 로고" /> | ||
| </a> | ||
| </h1> |
There was a problem hiding this comment.
👍 칭찬
로고에 h1 쓰신점 좋아요~
h1 태그는 해당 페이지의 제목과 같고 주로 로고에 사용한답니다~
| <nav> | ||
| <h1> | ||
| <a href="/" class="logo"> | ||
| <img src="/assets/logo.png" alt="판다마켓 로고" /> |
There was a problem hiding this comment.
💊 제안
img 태그의 alt 속성은 alternative 라는 의미로, 이미지 파일을 다운로드하는 것에 실패해서 이미지 파일을 보여줄 수 없을 때
어떤 이미지인지 파악할 수 있게 대신 제공되거나, 스크린리더로 읽혀지는 문자를 의미합니다.
따라서 가능한 해당 이미지를 잘 표현할 수 있는 문자열을 넣어주는 것이 좋습니다.
지금의 경우는 판다마켓이 대체 택스트로 더 적절할 것 같아요.
| <img src="/assets/logo.png" alt="판다마켓 로고" /> | |
| <img src="/assets/logo.png" alt="판다마켓"/> |
| * { | ||
| margin: 0; | ||
| padding: 0; | ||
| box-sizing: border-box; | ||
| ul, | ||
| ol { | ||
| list-style: none; | ||
| } | ||
| a { | ||
| text-decoration: none; | ||
| } | ||
| } |
There was a problem hiding this comment.
💊 제안
간략한 reset css 속성들이라 따로 분리하지 않으시고 한 파일에 작성하셨나봐요~
하지만 필요시 특정 태그의 스타일을 초기화하는건 번거로운 일이고, 이를 염두에 두고 css 작성을 해야하니 가능하면 처음부터 브라우저 스타일을 초기화하시고 reset.css로 분리하시고 작업하시는 것을 추천드립니다!
| @font-face { | ||
| font-family: "Pretendard-Regular"; | ||
| src: url("https://fastly.jsdelivr.net/gh/Project-Noonnu/[email protected]/Pretendard-Regular.woff") | ||
| format("woff"); | ||
| font-weight: 400; | ||
| font-style: normal; | ||
| } |
There was a problem hiding this comment.
❗️ 수정요청
html 상단에서 pretendard variable 폰트를 불러오셨는데, 이 부분이 필요할까요?
둘중 하나만 남기시면 좋겠어요~
그리고 가능하다면 가변 굵기를 지원하는 pretendard variable을 사용하시는 것을 추천드립니다!
| /* haeder 영역 */ | ||
| header { |
There was a problem hiding this comment.
👍 칭찬
파악하기 쉽게 관련된 css 들을 모아두시고 주석으로 구분해주신점 좋습니다!
| /* aria-disabled="true" 속성을 사용하여 버튼 비활성화 */ | ||
| .btn-login[aria-disabled="true"] { |
There was a problem hiding this comment.
💊 제안
아마 a 태그를 사용하셔서 일반적인 button 태그처럼 disable 될때를 대비해 이 속성을 추가해주신 것 같아요~
그런 경우를 대비하신 것은 좋지만, 해당 태그의 그런 요구사항이 아직 없으니 추후 필요할때 추가하시는 것이 좋지 않을까 하는 생각이 듭니다.
만약 link-button 과 같은, 범용적으로 사용하기위한 a 태그 class였다면 괜찮을 것 같은데 현재 login 버튼에서는 해당 기능이 사용되고 있지 않으니 오히려 헷갈릴 것 같아서요~
| background-color: #1251aa; | ||
| } | ||
|
|
||
| /* aria-disabled="true" 속성을 사용하여 버튼 비활성화 */ |
There was a problem hiding this comment.
💊 제안
주석을 다는 것은 좋은 습관입니다. 다만 주석또한 읽어야하는 정보라 중복되지 않는 내용을 입력해주시는 것이 가독성면에서 더 좋아요.
지금 주석은 아래 css 선택자를 통해 파악할 수 있는 내용이라 없어도 될 것 같습니다~
아니면 아래처럼 다른 개발자에게 추가적인 정보를 줄 수 있도록 작성하셔도 좋을 것 같아요.
/* 추후 사용될 수 있을 것 같아서 추가 */
/* 뭐뭐한 경우 해당 요소의 disabled가 필요해 추가 */
2 participants
요구사항
기본
심화
스크린샷
멘토에게