-
Notifications
You must be signed in to change notification settings - Fork 43
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
[이은주] sprint3 #99
The head ref may contain hidden characters: "Basic-\uC774\uC740\uC8FC-sprint3"
[이은주] sprint3 #99
Conversation
음 absolute으로 띄워야만 하는 이유가 있으신가요!? 굳이 absolute으로 띄울 필요가 없어보입니다 |
좋은 고민 영역입니다. 다만, 몇가지 이야기 해볼 수 있는게 지금처럼 디자인 시안에서 디자이너가 명시적으로 해당 스타일은 어떤 이름으로 칭합시다! 라고 이야기를 했다면 그 값에 대해서는 변수를 만들어주는게 좋아요. 만약 지금은 gray 700을 사용하고있다가, 디자이너가 gray 700을 800으로 일괄적으로 바꿔주세요라는 요청이 들어올 수도 있거든요. 그게 아니라면 개발자가 판단해서 임의로 변수화를 하면 좋답니다. |
이것도 고민을 많이 하셨을텐데요, 이제 어떤 값을 기준으로 마크업을 시작하셨는지에 따라 달라집니다. 데스크탑 스타일을 먼저 작업하셨다면 tablet / mobile 뷰에 대한 미디어 쿼리만 작업해주시면 될거에요. 그리고 readme에서 우리 마크업이 어떤 스타일을 기준으로 만들었는지 작성해준다면 다른 개발자들도 편하게 개발할 수 있겠죠 |
이 내용은 멘토링에서 이야기 해볼게요! |
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 관점에서 좋은 이름을 붙이는데에 어려워하시는게 보여요.
이거는 계속 블록 이름을 뭐라할지 고민하고 적용해나가다보면 개선되는 영역이랍니다.
좋은 방향으로 고민하고 계셔요.
이에 대한 자세한 리뷰는 코드단에 남겨두었으니참고 부탁드립니다.
고생 많으셨어요
@@ -50,10 +50,27 @@ | |||
font-weight: 100; | |||
} | |||
|
|||
* { | |||
body { | |||
font-family: "pretendard"; |
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.
font의 경우, pretendard는 외부에서 불러와 사용하는 폰트잖아요. 불러오는데에 실패하거나 오류가 발생한 경우, html은 브라우저에서 제공하는 못생긴 기본 폰트를 입혀서 제공해줍니다.
따라서 폰트 설정할때는 항상 fallback을 제공해줄 필요가 있어요
:root { | ||
--white: #ffffff; | ||
|
||
--gray-900: #1b1d1f; | ||
--gray-800: #26282b; | ||
--gray-600: #454c53; | ||
--gray-500: #72787f; | ||
--gray-400: #9ea4a8; | ||
--gray-200: #e5e7eb; | ||
--gray-100: #e8ebed; | ||
--gray-50: #f7f7f8; | ||
|
||
--blue1: #3692ff; | ||
--blue2: #1967d6; | ||
--blue3: #1251aa; | ||
--blue4: #cfe5ff; | ||
--blue5: #e6f2ff; | ||
|
||
--red: #f74747; | ||
} |
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.
변수들을 common으로 둔거 좋습니다!
만약 조금만 더 구조화를 욕심내어본다면 variables.css라는 녀석으로 분리해보면 어떨까요?
그리구, --gray-900
처럼 --color-###
형태로 갖고가고 계시잖아요?
이걸 계속 활용해서 --blue-900
형식으로 맞춰보면 어떨까요?
/* PC view */ | ||
@media screen { |
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-left: 50px; | ||
font-family: "ROKAF"; | ||
font-size: 26px; |
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에게 font-size: 10px 를 제공하고, 이후 요소들에게는 rem유닛을 한번 적용시켜보면 어떤가요?
.footer p, | ||
.footer 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.
BEM을 사용하면서는 굳이 child element에대한 설정을 하기보다 명시적으로 BEM 요소로 적용시키는게 좋지 않을까 생각합니다.
만약 block 요소 전체적으로 활용되어야 하는 스타일이라면 block에게 적용을 해보아도 좋겠어요
id="email" | ||
name="email" |
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.
id와 name 잘 설정해주셨어요!
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.
다만, 이 인풋은 이메일 값을 입력받기를 기대하고 있죠? 그런 관점에서 type=text
보다는 type=email
이 더 적합해보여요
<button | ||
class="form__password-hidden" | ||
aria-label="비밀번호 숨기기" | ||
></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.
요녀석의 경우, hide-password
라는 클래스로도 충분해보여요.
왜냐하면 form 그룹에 속해있긴 하지만 명확하게 비밀번호를 숨기기 위한 요소로 존재하잖아요.
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 작성해주신거 너무 좋습니다
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.
이 버튼이 input을 기준으로 위치가 잡혀야 할텐데요, 그 관점에서 input과 button을 감싸고 있는 래퍼를 하나 만들어서, absolute 포지셔닝을 잡아주면 어떨까요?
aria-label="비밀번호 숨기기" | ||
></button> | ||
</div> | ||
<button type="submit" class="form__button">로그인</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.
type=submit 설정 좋습니다
</div> | ||
<button type="submit" class="form__button">로그인</button> | ||
</form> | ||
<section class="social_login"> |
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.
로그인 페이지 피드백 한번 참고해서 여기도 적용해볼까요?
요구사항
기본
공통
랜딩 페이지
로그인, 회원가입 페이지 공통
심화
스크린샷
멘토에게
태블릿과 모바일의 미디어쿼리를 (min-width: 768px) and (max-width: 1199px)처럼 딱 지정한 너비에서만 css가 적용되게끔 했어요. 그런데 사진처럼 태블릿이 PC의 css를 내려받고, 또 모바일이 태블릿의 css를 내려받는 방식으로 하는 게 좋을까요?
지금은 루트폴더에 css, font, img폴더를 만들어서 파일들을 관리하고 있는데요. 이 방식이 맞을까요? html파일들만 폴더 밖으로 내놓는 게 맞는지, index.html을 제외한 페이지들의 html파일도 따로 폴더를 생성하여 그 안에 넣어 관리해야 하는지 궁금합니다.