-
Notifications
You must be signed in to change notification settings - Fork 7
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
[SP0] 메인 페이지 개선 #168
[SP0] 메인 페이지 개선 #168
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.
최고의 개발자 이혜준...👍
말씀 남겨주신 것에 대해서는 디자이너분들께서 왼쪽으로 치우친 디자인을 의도해주신 것 같아서 저도 비율로 가져가는게 맞을 것 같다고 생각이 드네요!
아니면 디자이너분들께 말씀드리고 구체적인 비율을 다시 물어보는건 어떨까요~?
고생하셨습니당!!
{currentTab.content.map(({ data, highlight }) => | ||
highlight ? <span>{data}</span> : data, |
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.
comment;
bold 처리된 부분을 highlight에 따라 처리한 것 인상깊네요!
h5 { | ||
font-weight: 400; | ||
color: #787878; | ||
} | ||
span { | ||
font-weight: 700; | ||
color: #ffffff; | ||
} |
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에서는 꼭 수정하지 않아도 괜찮고,
나중에 Text 스타일 관련한 공통 컴포넌트 만들어서 개선하는 방향으로 해보아요 ..!!!!
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.
서깅씨 SP1 하다가 궁금해져서 그런데 자식 선택자는 왜 지양하는 게 좋은가요?
자식 선택자를 지양하는 거면 위의 예에서는 h5, span 둘 다 styled-component로 만드는 게 좋은건가요?
border-radius: 12px; | ||
} | ||
|
||
cursor: ${({ isMobile }) => (isMobile ? 'pointer' : 'auto')}; |
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.
사실 모바일에서는 커서가 없기 때문에 따로 처리하지 않아도 괜찮습니다 ..!!!!!
@@ -50,7 +50,7 @@ export function DetailedInformation() { | |||
> | |||
<div className={`${styles.nameWrapper} ${GTM_CLASS[`informationCard${name}`]}`}> | |||
<h4 className={`${styles.name} ${GTM_CLASS[`informationCard${name}`]}`}>{name}</h4> | |||
<ArrowRight /> | |||
<img className={`${styles.arrow}`} src={ArrowRight} 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.
이것 사실은 {${}
} 로 안 감싸고 {} 안에만 넣어도 괜찮긴 할텐데 ..!! 다른 데에서도 이렇게 했으니 일단 글케 해도 괜찮을 것 같아요 ..!!!!
imgSrc: seminarImage, | ||
content: | ||
'활동 기간 동안 총 8회의 파트별 세미나를 통해 각자 자신의 파트에서 실력을 다져요. 각 파트장의 강연, 파트원간의 지식 공유, 외부 연사 초청 등 다양한 유형의 세미나가 진행돼요.', | ||
imgSrc: imgSeminar, |
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.
요것 괜찮다면 ./constant.ts 만들어서 그곳에 옮겨도 좋을 것 같아요 ..!!
content: | ||
'활동 기간 동안 총 8회의 파트별 세미나를 통해 각자 자신의 파트에서 실력을 다져요. 각 파트장의 강연, 파트원간의 지식 공유, 외부 연사 초청 등 다양한 유형의 세미나가 진행돼요.', | ||
imgSrc: imgSeminar, | ||
content: [ |
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.
요렇게 구조 짠 것 너무너무 좋아요!
요것이 렌더되면 결과로 textnode가 생기는 것이니까,
content => nodes
data => textContent
요런 식으로 변수 이름을 바꾸어 보면 어떨까요.!?
Summary
Screens
Comment
360px
떨어진 지점에 있습니다. 근데 이게1920px
기준의 디자인이라 작은 화면으로 보면 거의 가운데 정렬과 유사하게 보이더라고요. 그래서 left 값을 아예 고정된 값인360px
대신 화면 너비에 따라 좀 변동되도록18.8vw
를 줬는데 뭔가 더 좋은 방법이 있을까요?