Skip to content
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

feat: implement intro section component #95

Merged
merged 3 commits into from
Jul 3, 2024
Merged

feat: implement intro section component #95

merged 3 commits into from
Jul 3, 2024

Conversation

sounmind
Copy link
Member

@sounmind sounmind commented Jul 3, 2024

Checklist before merging

  • Link an issue with the pull request
  • Ensure no errors or warnings on the browser console
  • Avoid additional major pushes after approval (if necessary, request a new review)

@sounmind sounmind requested a review from a team as a code owner July 3, 2024 20:52
@sounmind sounmind linked an issue Jul 3, 2024 that may be closed by this pull request
@sounmind
Copy link
Member Author

sounmind commented Jul 3, 2024

image image image

@@ -20,6 +20,7 @@ class Hero extends HTMLElement {
h2 {
font-weight: inherit;
font-size: 30px;
word-break: keep-all;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SamTheKorean 단어 단위의 줄바꿈을 위해 hero 컴포넌트에 이 속성을 추가했어요

Comment on lines 23 to 33
background: linear-gradient(
0deg,
rgba(255, 255, 255, 0.7) 0%,
rgba(255, 255, 255, 1) 100%
),
linear-gradient(
90deg,
rgba(36, 234, 202, 0.8) 0%,
rgba(132, 109, 233, 0.8) 100%
);
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nhistory 이렇게 하는 것이 세환님이 의도하신 걸까요?

Comment on lines +74 to +92
@media only screen and (min-width: 768px) {
article {
/* NOTE: Requires accurate policy settings */
row-gap: -3.2px;
}
}

@media only screen and (min-width: 1024px) {
article {
row-gap: unset;
width: 40%;
}
}

@media only screen and (min-width: 1024px) {
aside {
width: 60%;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

개인적으로는, 미디어쿼리를 아래와 같이 통합하는게 가독성에 더 좋다고 생각하는데, 혹시 각 엘리먼트마다 구분해서 미디어쿼리를 작성하신 이유가 있으실까요?

@media only screen and (min-width: 768px) {
  .inner-container {
    ...
  }
  article {
    ...
  }
}

@media only screen and (min-width: 1024px) {
  .inner-container {
    ...
  }
  article {
    ...
  }
  aside {
    ...
  }
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이렇게 작성하면 각 엘리먼트, 선택자 별로 어떻게 반응형으로 대응하는지 빠르게 알 수 있는 것을 경험했기 때문에 선호하는 방식이에요.
예를 들어, 미디어쿼리 안에 스타일 코드가 길어질 경우, 특정 엘리먼트에 대한 반응형 스타일을 미디워쿼리 별로 비교하면서 스타일 코드를 작성할 때 매번 스크롤을 많이 이동해야 하는 불편함을 경험했었거든요.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이것도 스타일 코드 양이 많아질수록 dx가 개선될 수 있는 좋은 안이 될 수 있을 것 같네요!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sounmind @yolophg What an interesting conversation!

2024년이니 저 같으면 미디어 쿼리를 선택자 안에 둘 것 같아요! 이 방식이 우리가 실제 스타일 생각하는 방식과 제일 유사히지 않나요?

article {
  ...

  @media only screen and (min-width: 768px) {
    ...
  }

  @media only screen and (min-width: 1024px) {
    ...
  }
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 이런 문법이 가능했었군요? 몰랐습니다. 이게 가능하다면 제가 가장 선호하는 방식이 되겠네요!!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DaleSeo 이 스펙에 관련된 문서를 어디서 찾을 수 있을까요??

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

너무 최신이라 하이라이팅이 못 따라가네요...ㅋㅋㅋ

@yolophg
Copy link
Contributor

yolophg commented Jul 3, 2024

수고하셨습니다!

@sounmind sounmind force-pushed the 94-intro-section branch from b58b01a to fc4533f Compare July 3, 2024 23:53
@sounmind sounmind merged commit e0785b3 into main Jul 3, 2024
@sounmind sounmind deleted the 94-intro-section branch July 3, 2024 23:54
createHtml() {
return html`
<section>
<div class="inner-container">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

상위 section 요소에 바로 스타일을 해도 되지 않을까요? 현재 section에 적용되어 있는 스타일은 :host psedo class 선택자로 적용해주고요. 이러한 패턴을 다른 PR에서 본 것 같은데, 불필요한 div 요소를 줄일 수 있어서 좋은 것 같았습니다.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오? 그랬군요. 알아보고 가능하다면 적용해보겠습니다.

@@ -25,6 +25,10 @@ html {
scroll-behavior: smooth;
}

body {
margin: 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 글로벌 스타일 때문에 밑에 섹션들이 모두 숨막히게 붙어버리는 좌우로 붙어버리는 것은 같은데 의도하신 건가요?

Shot 2024-07-03 at 19 35 09@2x

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네 의도한 것 맞습니다.
지금 body에 부여된 스타일로는 section 컴포넌트들을 요구사항대로 구현할 수 없었기 때문에...(배경 때문이라도)
일단 초기화 해놓았습니다.
global style 관련해서 오늘 정기미팅 안건에 올려놓았는데요. 이야기해보면 좋겠습니다.

Comment on lines +74 to +92
@media only screen and (min-width: 768px) {
article {
/* NOTE: Requires accurate policy settings */
row-gap: -3.2px;
}
}

@media only screen and (min-width: 1024px) {
article {
row-gap: unset;
width: 40%;
}
}

@media only screen and (min-width: 1024px) {
aside {
width: 60%;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sounmind @yolophg What an interesting conversation!

2024년이니 저 같으면 미디어 쿼리를 선택자 안에 둘 것 같아요! 이 방식이 우리가 실제 스타일 생각하는 방식과 제일 유사히지 않나요?

article {
  ...

  @media only screen and (min-width: 768px) {
    ...
  }

  @media only screen and (min-width: 1024px) {
    ...
  }
}

@DaleSeo DaleSeo mentioned this pull request Jul 4, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

intro-section
3 participants