-
Notifications
You must be signed in to change notification settings - Fork 79
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
[이종욱] sprint2 #87
The head ref may contain hidden characters: "part1-\uC774\uC885\uC6B1-sprint2"
[이종욱] sprint2 #87
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 잘 봤습니다! 이번주도 좋은 시도들이 아주 많이 보인 PR이었습니다 :)
특히 eye toggle 처리하신 부분.. 처음 하시는 분들은 학습의 의미로 다 js로 해결하려하시는데 사실 이런 경우 js로 해결하는것보다는 css만으로 해결하는게 가장 부하도 적고 나은 방법이라서, 칭찬드리고싶네요! ㅎㅎ 몇가지 리팩터링 진행하실 때 참고하시면 좋을만한게 보여서 코멘트 드려봤어요
이번주도 수고하셨습니다!! 👍
flex-direction: column; | ||
position: relative; |
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.
position: relative
써주고 바로 상대적인 위치를 써주는 코드가 많이 보이네요! padding이나 margin 으로도 해결할수있지않을까요?
</section> | ||
<section class="panda-section left-start"> | ||
<div> | ||
<img src="/assets/images/panda3.svg" /> | ||
<img src="/assets/images/panda3.svg" alt="판다마켓 내용1"/> |
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.
alt text 써준것만으로도 합격...! 이지만 ㅋㅋㅋ 내용 조금 디테일하게 넣어주시는게 좋을것같아요~
보통 이미지를 설명하는 용도로 내용을 많이 써줍니다!
display: flex; | ||
width: 100%; | ||
gap: 10px; | ||
height: 33.75rem; |
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.
음 rem계산하기쉽게 default font size를 10px 변환 후 사용하는건..어때요? ㅎㅎ
html 태그의 font-size
조절해주시면됩니다!
} | ||
|
||
.panda.intro { | ||
padding-inline: 404px 195px; | ||
.panda-banner.top-banner > img { |
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.
개인적으로 이 이미지는 전체 컨테이너를 기준으로 (panda-banner) 위치 잡아주는게 제일 깔끔하고, 의도한대로 동작할것같아요. 이미지가 항상 .panda-banner
의 오른쪽 끝에서 223px 떨어져있는것같은데요?
그리고 top-banner와 bottom-banner를 구분지어 클래스이름을 준것도 유의미하지않은것같습니다.
두 배너 다 컨테이너를 기준으로 텍스트가 시작하는 위치, 이미지가 들어간 위치, 정렬된 기준이 동일하기때문에 같은 클래스이름으로 묶어주는것이 오히려 바람직해보여요 :) 참고하셔서 리팩터링 진행해보세요!
@@ -75,28 +77,26 @@ <h3> | |||
</p> | |||
</div> | |||
</section> | |||
<section class="panda banner"> | |||
<section class="panda-banner bottom-banner"> |
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.
BEM 규칙을 따르자면 main-banner__bottom
정도가 될것같아요.
지금 경우를 예시를 들자면 main
은 block(컨테이너 블락), banner
는 element(블락의 하위요소), bottom
은 modifier(수정자 즉 variation)로 볼수있겠죠?
@@ -6,10 +6,39 @@ | |||
content="width=device-width, user-scalable=no, initial-scale=1.0, maximum-scale=1.0, minimum-scale=1.0"> | |||
<meta http-equiv="X-UA-Compatible" content="ie=edge"> | |||
<title>로그인 페이지</title> | |||
<link href="/styles/reset.css" rel="stylesheet" type="text/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.
css 로드하는 순서에 따라서도 우선순위 적용이 바뀌는데, 잘 적용해보셨네요 👍
<h2>반갑습니다.</h2> | ||
<h3>처음 뵙겠습니다.</h3> | ||
<header> | ||
<img src="/assets/images/signupPage/pandamarket-logo.svg" alt="판다마켓 로고"> |
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태그 안에 svg파일을 두셨네요!
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에 드린 코멘트인데, 아래 아티클보시고 svg에 대해 자세히 개념 정리해보시면서 연습삼아 inline svg vs img 태그의 src로 쓴 svg가 유의미한 성능차이가 나는지 디버깅해보실래요~?
<input type="checkbox" id="password-toggle-confirm" hidden> | ||
<label for="password-toggle-confirm" class="toggle-switch"></label> | ||
</div> | ||
<button id="panda-market-signup-button" type="submit">회원가입</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.
button type submit까지 적용해두신거 굳굳! 디테일 👍
} | ||
|
||
:root { | ||
--banner-bg-color: #CFE5FF; |
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.
음 개인적으로 여기 정의된 컬러들의 이름은 쓰인 ui요소를 기준으로 만든다기보다는 디자이너가 지정한 컬러이름이 있다면 해당 컬러 이름으로 써주거나, 종욱님이 생각했을때 해당 컬러가 주로 쓰이는 컬러인지 or 보조역할로 쓰이는 컬러인지 or 시스템의 상태를 나타내는 컬러인지 구분 후 목적과 용도에 따라 네이밍해주는게 정확하다는 생각이 드네요!
디자인 시스템에서 컬러 네이밍 정하기 읽어보시면 어떤 기준으로 컬러 이름이 만들어지는지 참고하기 좋고, 나중에 디자이너와 협업하실때 도움 많이 되실것같아요 👍
cursor: pointer; | ||
} | ||
|
||
#password-toggle:checked + .toggle-switch, |
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.
👍 js에서 조작하는것말고 css로 해결할수있으면 best죠! checked 속성 이용해서 깔끔하게 잘 해결하셨네요 굳굳!! 👍
멘토의 답변멘토님께 개인 DM으로 메인 페이지 내 이미지 크기로 인한 불필요 여백 발생 문제를 해결하고자종횡비 및 비율을 통해 해결하고자 하였으나, 속성 및 비율 적용 이후에도 비슷한 문제가 발생되어 원인을 찾지 못해 비슷한 문제 어떤게 있는걸까요~? |
요구사항
기본
- “https://www.google.com/”,
- “https://www.kakaocorp.com/page/”
심화
주요 변경사항
스크린샷
https://sprintwook.netlify.app/
멘토에게
종횡비 및 비율을 통해 해결하고자 하였으나, 속성 및 비율 적용 이후에도 비슷한 문제가 발생되어 원인을 찾지 못해
추후 다시 피드백을 통해 검토를 해야 될 듯 합니다.