-
Notifications
You must be signed in to change notification settings - Fork 39
정새론 sprint1 #1
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 #1
The head ref may contain hidden characters: "Basic-\uC815\uC0C8\uB860-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.
전반적으로 깔끔하게 잘 작성하셨네요. 수고하셨습니다! 👍
NIT는 리뷰를 반영하실필요없고 학습적인 목적으로 작성된 코멘트라 참고해보시면됩니다.
주요 리뷰 포인트
- 접근성을 고려한 언어 설정
- 모호한 클래스명 수정하기
- 시맨틱 태그 활용도 높이기
- 불필요한 div 중첩 줄여보기
- 디폴트 값 사용 줄이기
- 전역 스타일 정의하고, css 변수 만들어서 재사용성 높이기 (코드 중복 줄이기)
index.html
Outdated
| <div class="card"> | ||
| <div class="card-content-rev"> |
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 중첩이 발생되는것 같아요.
div class ="card card-content-rev" 이렇게 한단계로 줄여볼까요?
질문에 대한 답변심화과정은 Margin을 %나 auto로 설정해서 적용해야 하나요?margin말고도 다른 속성에도 가변 단위를 꼭 사용할 필요는 없지만 반응형을 고려했을때는 가변 단위를 사용하시는게 좀 더 적합합니다. 만들면서 Grid를 한 번도 안 쓰고 전부 Flexbox로 위치를 줬는데, 이렇게 해도 맞게 된 건가요?CSS Grid/ Flexbox를 어떤 이유로 어느때 더 사용하기 적절한지 찾아보시면 좋을 것 같아요 :) 중간에 카드 3개의 너비가 약간 다른 것을 알 수 있습니다. 어떻게 수정해야 할 지 모르겠습니다ㅠㅠ카드의 최대 너비(max-width)를 지정하고, |
요구사항
기본
심화
스크린샷
멘토에게