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] 헤더 상단 고정 및 z-index 수정 #127

Merged
merged 3 commits into from
Nov 21, 2023

Conversation

LEEJW1953
Copy link
Collaborator

@LEEJW1953 LEEJW1953 commented Nov 21, 2023

설명

완료한 기능 명세

  • 헤더 position : fixed 적용
  • z-index : 999 적용

스크린샷

_2023_11_21_16_42_32_614.mp4

리뷰 요청 사항

특별히 리뷰해 주었으면 하는 부분, 고민되는 부분 기재하기

@@ -3,10 +3,14 @@ import { style } from '@vanilla-extract/css';
import { vars, fontStyle } from '@/styles';

export const container = style({
position: 'sticky',
Copy link
Collaborator

@js43o js43o Nov 21, 2023

Choose a reason for hiding this comment

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

P3: 'fixed' 대신 'sticky'를 쓰신 이유가 궁금합니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

음 생각해보니 fixed를 사용하면 html 높이 속성을 바꿀 필요가 없을 수도 있겠네요 수정하겠습니다!

Copy link
Collaborator

Choose a reason for hiding this comment

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

넵 지금 minHeight, 100vh 써보는데도 똑같은 현상이 일어나서요..😂 좋습니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

28e9042
수정하였습니다

Copy link
Member

@ttaerrim ttaerrim left a comment

Choose a reason for hiding this comment

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

스크린샷 추가해 주세요~~ 영상으로요 👀

borderBottom: `1px solid ${vars.color.grayscale100}`,
zIndex: '999',
Copy link
Member

Choose a reason for hiding this comment

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

p3; 뭔가 zIndex도 관리하고 싶다는 생각이 드네요.. 찾아보았는데 변수로 관리하는 솔루션도 있네요! (아마 sass?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

오 이런 규칙? 도 있군요

Copy link
Collaborator Author

@LEEJW1953 LEEJW1953 left a comment

Choose a reason for hiding this comment

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

25b9eab
스크린샷과 헤더에 가려지는 부분 처리를 위한 css 추가하였습니다

Copy link
Member

@ttaerrim ttaerrim left a comment

Choose a reason for hiding this comment

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

스크린샷 없어요 ㅠ 아무튼 확인했습니다

Copy link
Collaborator Author

@LEEJW1953 LEEJW1953 left a comment

Choose a reason for hiding this comment

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

분명 재생되는 것 까지 확인했었는데 수정 완료를 안눌렀나보네요;; 추가했습니다!

@LEEJW1953 LEEJW1953 merged commit cdeb1fe into boostcampwm2023:develop Nov 21, 2023
1 check passed
@LEEJW1953 LEEJW1953 deleted the fe-header branch November 21, 2023 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Header 컴포넌트 구현
3 participants