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 : made hero component responsive #49

Merged
merged 7 commits into from
Jun 22, 2024
Merged

feat : made hero component responsive #49

merged 7 commits into from
Jun 22, 2024

Conversation

SamTheKorean
Copy link
Contributor

@SamTheKorean SamTheKorean commented Jun 14, 2024

  • 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)

@SamTheKorean SamTheKorean self-assigned this Jun 14, 2024
@SamTheKorean SamTheKorean linked an issue Jun 14, 2024 that may be closed by this pull request
@SamTheKorean
Copy link
Contributor Author

SamTheKorean commented Jun 14, 2024

@nhistory 각각 버전들마다 정확히 어디까지 breakpoint를 잡을 지 현재 피그마 디자인만 봤을 때는 확신이 안들고 이 부분은 컴포넌트마다 통일이 필요할 것 같아서 우선 1024px와 480px을 max-width로 잡고 진행하였습니다. 혹시 따로 의도하신 부분이 반영이 안됐다면 피드백 부탁드립니다!

@SamTheKorean SamTheKorean force-pushed the sam/48 branch 2 times, most recently from d120e03 to ba4fe19 Compare June 14, 2024 11:20
@nhistory
Copy link
Contributor

nhistory commented Jun 14, 2024

넵 제가 생각하는 breakpoint 구성은 아래와 같습니다.

image

커브드 모니터나 해상도가 큰 화면을 사용하는 경우 페이지 뷰가 어그러지는 경우가 종종 있어서
extra large 옵션을 추가하였습니다.

만약 모바일 뷰를 디폴트로 한다고 하면 미디어 쿼리 코드는 아래와 같이 구분할 수 있을 것 같습니다.

/* Default: Extra-small devices such as small phones (less than 640px) */

/* Medium devices such as tablets (768px and up) */
@media only screen and (min-width: 768px) {...}

/* Large devices such as laptops (1024px and up) */
@media only screen and (min-width: 1024px) {...}

/* Largest devices such as desktops (1280px and up) */
@media only screen and (min-width: 1280px) {...}

@nhistory nhistory mentioned this pull request Jun 14, 2024
11 tasks
@nhistory
Copy link
Contributor

nhistory commented Jun 14, 2024

@SamTheKorean 미디어 쿼리 포맷에 관해서 다같이 논의 해보는 게 좋을 것 같아서 Meeting4 #43 Agenda에 추가하였습니다.

components/hero.js Outdated Show resolved Hide resolved
components/hero.js Outdated Show resolved Hide resolved
components/hero.js Outdated Show resolved Hide resolved
@sounmind
Copy link
Member

넵 제가 생각하는 breakpoint 구성은 아래와 같습니다.

image

커브드 모니터나 해상도가 큰 화면을 사용하는 경우 페이지 뷰가 어그러지는 경우가 종종 있어서 extra large 옵션을 추가하였습니다.

만약 모바일 뷰를 디폴트로 한다고 하면 미디어 쿼리 코드는 아래와 같이 구분할 수 있을 것 같습니다.

/* Default: Extra-small devices such as small phones (less than 640px) */

/* Small devices such as large phones (640px and up) */
@media only screen and (min-width: 640px) {...}

/* Medium devices such as tablets (768px and up) */
@media only screen and (min-width: 768px) {...}

/* Large devices such as laptops (1024px and up) */
@media only screen and (min-width: 1024px) {...}

/* Largest devices such as desktops (1280px and up) */
@media only screen and (min-width: 1280px) {...}

@nhistory 세환님. 현재 피그마 디자인에서는 3가지만 보이는데 앞으로 더 추가하실 계획인가요?

@nhistory
Copy link
Contributor

@sounmind 네 Evan님 Largest 옵션은 따로 피그마 디자인은 추가하지 않을 생각이고요. 일단 Desktop 디자인을 그대로 적용할 생각입니다.
max-width 설정을 사용하면 대부분 큰 모니터에서도 별다른 문제 없이 화면 구현이 될 거라 생각하는데
혹시 테스팅 과정에서 이슈가 발생하면 추가적인 옵션을 추가하면 좋을 것 같습니다.

@SamTheKorean SamTheKorean requested a review from a team as a code owner June 19, 2024 13:25
@SamTheKorean SamTheKorean force-pushed the sam/48 branch 5 times, most recently from db05468 to 1be9728 Compare June 20, 2024 12:10
@SamTheKorean
Copy link
Contributor Author

변경된 코딩스타일과 정해진 media query에 따라서 코드 업데이트 했습니다! 해당 컴포넌트에서는 validate 함수와 default 설정 함수를 쓸 지 의문이 들어 적용하지 않았는데 혹시 필요하다고 보시면 피드백 부탁드립니다!

Copy link
Contributor

@DaleSeo DaleSeo left a comment

Choose a reason for hiding this comment

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

해당 컴포넌트에서는 validate 함수와 default 설정 함수를 쓸 지 의문이 들어 적용하지 않았는데 혹시 필요하다고 보시면 피드백 부탁드립니다!

속성이 children 밖에 없어서 딱히 유효성 검증이나 기본값 설정할 게 없을 것 같은데요?

h2 {
font-weight: inherit;
font-size: 30px; /* Default font-size for h2 */
display: block; /* Ensure block display */
Copy link
Contributor

Choose a reason for hiding this comment

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

이러한 Obvious한 코멘트는 지양합니다. 그리고 <h2> 요소의 display 속성은 원래 기본값이 block 아닌가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이런 코멘트도 리뷰어의 생산성을 떨어뜨릴 수 있겠군요. 피드백 반영하겠습니다!

@SamTheKorean SamTheKorean merged commit 05e036f into main Jun 22, 2024
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.

Make Hero Component Responsive
4 participants