-
Notifications
You must be signed in to change notification settings - Fork 39
[염휘건] sprint1 #33
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 #33
The head ref may contain hidden characters: "Basic-\uC5FC\uD718\uAC74-sprint1"
Conversation
addiescode-sj
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.
수고하셨습니다!
리드미도 잘 작성해주시고 전체적으로 좋은 시도 많이 해보셨네요 :)
피드백 드린거 반영해서 조금만 더 다듬어봅시다!
주요 리뷰 포인트
- 클래스이름 수정
- 구조화
- 포맷팅
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.
굳굳 👍 README를 잘 작성하고 관리하는걸 습관으로 들여보시면 좋아요
| @@ -0,0 +1,100 @@ | |||
| <!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="kr"> | |
| <html lang="ko"> |
kr이 아닌 ko가 올바른 문법입니다! :)
| <body> | ||
| <header> | ||
| <div class="wrap"> | ||
| <a href="/" class="logo pointer"> |
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.
접근성 향상을 위해 aria-label="홈으로 이동" 과 같이 관련있는 요소에 aria-label 혹은 role을 사용해볼까요?
아래 아티클 참고해보시고 접근성을 향상시키기 위해 어떤 노력을 할수있는지 더 찾아보시면 좋은 공부 될 것같아요 :)
https://joyhong-91.tistory.com/22
https://velog.io/@a_in/WAI-ARIA-role-aria-label
|
|
||
| </header> | ||
| <main> | ||
| <section class="top landing"> |
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.
클래스이름에 위치, 색상, 순서 등의 키워드가 들어가있는것은 다소 좋지않아요. 디자인이 변경되면 같이 변경되어야겠죠? 그러면 기존에 애써 구조화해놨던 위계가 무너질수도있고요. 클래스이름을 좀 더 엘리먼트의 역할에 맞게 고쳐볼까요?
| </div> | ||
| </section> | ||
| <section class="section"> | ||
| <div class="wrap left"> |
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.
요 부분도 wrap left 보다는, wrap reverse와 같이 방향을 바꾼 엘리먼트라는걸 명확히 나타내주시는게 디자인 변경에 대응하기도 쉽고 의미도 직관적으로 바뀔수있겠죠? :)
| :root { | ||
| --background-skyblue: #CFE5FF; | ||
| --background-litegray: #FCFCFC; | ||
| --primary-100: #3692FF; | ||
| --secondary-200: #E5E7EB; | ||
| --secondary-400: #9CA3AF; | ||
| --secondary-700: #374151; | ||
| --secondary-900: #111827; | ||
| } | ||
| @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; | ||
| } | ||
|
|
||
| * { | ||
| box-sizing: border-box; | ||
| } | ||
| html { | ||
| font-family: 'Pretendard-Regular'; | ||
| font-size: clamp(12px, 1.6vw, 16px) | ||
| } | ||
| body { | ||
| margin: 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.
css 파일을 용도에 맞게 분리해서 관리해볼까요?
- reset.css: 스타일 초기화
- common.css: 전역 스타일 처리
| header { | ||
| position: fixed; | ||
| top:0; left: 0; right: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.
불필요한 공백은 없애주세요!
| @@ -0,0 +1,234 @@ | |||
| :root { | |||
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.
컬러코드뿐만이 아니라, 레이아웃이나 여백에 관련해 자주 사용되는 값들도 변수로 만들어 재사용해주면 좋을것같아요 :)
예시)
/* Spacing */
--spacing-xs: 0.5rem;
--spacing-sm: 1rem;
--spacing-md: 2rem;
--spacing-lg: 4rem;
--spacing-xl: 8rem;
/* Border Radius */
--border-radius-sm: 8px;
--border-radius-md: 12px;
--border-radius-lg: 40px;
/* Container */
--container-max-width: 1920px;
--container-padding: 200px;| --secondary-700: #374151; | ||
| --secondary-900: #111827; | ||
| } | ||
| @font-face { |
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.
좋은 시도입니다!
폰트를 직접 로컬에서 관리하면 여러가지 최적화를 시도하실수있어요.
아래 추천드린 아티클 보시고
폰트 최적화 방법도 더 알아가시고, 어떤 방식으로 폰트를 보여줄지도 정해보고, 서브셋도 활용해볼까요? :)
|
|
||
| /* main contents */ | ||
| main { | ||
| width: 100%; |
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.
음 전체적으로 들여쓰기가 4칸씩 되어있는데 2칸이면 충분합니다.
하지만 일괄적으로 바꾸실수없으니 일단 지금은 냅두고, 나중에 prettier 플러그인을 설치해 일관적으로 포맷팅을 자동화할수있게끔 시도해봐요 :)
prettier가 어떤 도구이고 무엇을 할수있는지 소개해드릴게요!
참고
질문에 대한 답변
네, 브라우저가 항상 고정된 사이즈로 사용하는게 아니다보니 반응형을 고려한다면 미디어쿼리를 사용해 각 분기점 마다, 그리고 분기점 사이에서 유연하게 디자인 의도를 반영할수있도록하시는게 좋습니다. 다음 미션때는 반응형 고려해서 작업해봐요! |
요구사항
기본
심화
주요 변경사항
.landing클래스는 하위 박스인.wrap에서,max-width와margin: 0 auto를 활용해서 하늘색 배경색은 너비를 꽉 채우도록 채워지고, 내부 요소들의 위치는 고정되고, 여백만 커지도록 구현했습니다.<a>태그로 감싸서, 클릭시 특정 페이지들로 이동할 수 있습니다.vw를 활용해서 동적으로 크기가 조절됩니다.target="_blank" rel="noopener"속성을 사용하여, 보안상 취약점이 발생하고 퍼포먼스가 저하되는 문제를 해결했습니다.스크린샷
배포 링크
멘토에게
미디어쿼리를 사용하지 않고 제작하여 화면이 줄어들면 의도치 않은 디자인의 변경이 이루어집니다.
figma를 기준으로 제작했을때 여백이나 위치 등 css를 어느 수준가지 일치시켜야 하는지 기준이 궁금합니다.
코드의 개선이 가능하다면 어떤 점을 개선해야할지 궁금합니다.
셀프 코드 리뷰를 통해 질문 이어가겠습니다.