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

[Team-33][bangtae & 포키] 1주차 금요일 PR #94

Merged
merged 4 commits into from
May 29, 2022

Conversation

bangdler
Copy link

안녕하세요 후~

1주차 금요일 PR 입니다.
진행 사항이 많지 않지만 잘 부탁드립니다.

진행 사항

  • GNB, Main Banner, Search bar UI 구현
  • Search bar 포커스 및 블러 이벤트 추가
  • react-app-rewired 를 이용한 절대경로 적용

image

고민했던 점

  • Search bar 에 체크인과 같은 각 영역 클릭 시 css 변경 기능 구현 시 접근성을 고려하여 focus 이벤트를 적용하였습니다.

진행 예정

  • Search bar 기능 구현
  • 캘린더 모달 구현

bangdler and others added 4 commits May 26, 2022 12:41
* rename: gnb 폴더 생성하여 관련 컴포넌트 이동
   - styled -> customStyled 로 변경 적용
* refactor: gnb 컴포넌트 css 수정
* feat: main banner UI 구현
* refactor: gnb 컴포넌트 hover 및 cursor pointer 적용
* chore: mui 버전 통일

* feat: 글로벌 스타일 추가

* feat: SearchBar 컴포넌트 style 작성 및 이벤트 추가
* chore: alias 경로 설정을 위한 react-app-rewired 설치 및 설정파일 생성
* refactor: 절대경로 반영 (index, app, gnb, main-banner)
* fix: search input width 고정
@moonyerim2 moonyerim2 added the review-FE New feature or request label May 27, 2022
Copy link

@choisohyun choisohyun left a comment

Choose a reason for hiding this comment

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

안녕하세요~ bangtae & 포키!
리뷰어 후입니다.
캡쳐해주신 레이아웃으로 봤을 때 깔끔하게 구현 잘하신 것 같습니다.

접근성, 시맨틱 마크업만 제대로 지켜도 lighthouse 성능이나 편의성이 늘기 때문에 이러한 시도들 너무 좋아 보입니다!!
저는 코쿼에서 미션할 때 접근성 관련되어서는 거의 신경을 못썼는데 반성하게 되네요 ㅠㅠㅎㅎ
다음에 데모도 올려주신다면 UI나 동작들도 살펴보도록 하겠습니다~
수고많으셨습니다~

@@ -0,0 +1,5 @@
const { alias, configPaths } = require('react-app-rewire-alias');

Choose a reason for hiding this comment

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

절대경로를 적용하셨군요! 👍
cra를 사용하지 않는 경우에는 별도 의존성 추가하지 않아도 사용하는 번들러(webpack, vite 등) config 설정만으로도 절대경로 설정이 가능합니다~ 참고만 해주세요 ㅎㅎ

width: 88px;
height: 46px;
line-height: 46px;
text-align: center;
font-size: ${({ theme }) => theme.fontSize.logo};
font-weight: ${({ theme }) => theme.fontWeight.logo};
color: ${({ theme }) => theme.color.grey1};
cursor: pointer;

Choose a reason for hiding this comment

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

h1 하위로 Link(react router dom) 컴포넌트로 링크 거는식으로 추가해봐도 좋을 것 같습니다~
저는 보통 cursor: pointer는 버튼이나 링크같은 경우에만 사용하는거 같아서 이질감이 조금 있는거 같네요

top: 0px;
width: 1440px;
height: 640px;
z-index: -1;

Choose a reason for hiding this comment

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

z index를 마이너스로 두는것도 방법이겠지만, background image로도 넣을 수 있겠네요!

</IconButton>
);
}

const IconButton = styled.button`
visibility: ${({ visibility }) => visibility};

Choose a reason for hiding this comment

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

visibility와 display의 차이점도 참고해 보시면 좋을것 같습니다~

https://unabated.tistory.com/entry/displaynone-%EA%B3%BC-visibilityhidden-%EC%9D%98-%EC%B0%A8%EC%9D%B4

label={label}
placeholder={placeholder}
isLastElement={isLastElement(index) ? true : false}
isCurrentInput={isCurrentInput(label) ? true : false}

Choose a reason for hiding this comment

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

Suggested change
isCurrentInput={isCurrentInput(label) ? true : false}
isCurrentInput={isCurrentInput(label)}

boolean을 반환하는 함수일 경우에는 또 true/false로 변환해 주지 않아도 됩니다!

Comment on lines +20 to +23
<SearchMenu bgColor={isFocus ? 'grey6' : 'white'} tabIndex="0" onBlur={handleBlur}>
{searchInputText.map(([key, { label, placeholder }], index) => (
<Fragment key={key}>
<SearchInput

Choose a reason for hiding this comment

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

span(SearchMenu) 태그는 div 태그(SearchInput)를 포함할 수 없습니다!
아마 react에서도 워닝이 떴을 것 같은데 확인 한번 해보시면 좋을것 같습니다.

https://dasima.xyz/div-vs-span-vs-p-%EC%B0%A8%EC%9D%B4%EB%8A%94-%EB%B8%94%EB%A1%9D-%EC%9A%94%EC%86%8C%EC%99%80-%ED%8F%AC%ED%95%A8-%EC%9C%A0%EB%AC%B4/

Comment on lines +25 to +26
{isLastElement ? null : <Line />}
{isLastElement ? <SearchButton isFocus={isFocus} /> : null}

Choose a reason for hiding this comment

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

Suggested change
{isLastElement ? null : <Line />}
{isLastElement ? <SearchButton isFocus={isFocus} /> : null}
{isLastElement ? <SearchButton isFocus={isFocus} /> : <Line />}

border-left: 1px solid ${({ theme }) => theme.color.grey5};
`;

const Container = styled.div`

Choose a reason for hiding this comment

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

상위 태그 스타일부터 순서대로 위치하면 보기에 좀더 편할것 같네요~

@choisohyun choisohyun merged commit fd418d3 into codesquad-members-2022:team-33 May 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-FE New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants