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

[고급 레이아웃 컴포넌트] 가브리엘(윤주현) 미션 제출합니다. #62

Merged
merged 92 commits into from
Oct 2, 2023

Conversation

gabrielyoon7
Copy link
Member

@gabrielyoon7 gabrielyoon7 commented Sep 25, 2023

🎯 2단계. 고급 레이아웃 컴포넌트

공통

  • 의미있는 커밋 메시지 작성
  • PR에 관련 없는 변경 사항이 포함 유무 확인

컴포넌트 요구사항

  • 선택한 컴포넌트의 기능 구현
  • npm 배포
  • README.md에 코드 저자로서 특히 고민한 부분과 의도 설명
  • 테스트 코드를 작성하였나요? (선택 사항)

📌 구현한 내용 설명

SplitPane을 구현하였습니다.

이번 미션에서도 지난 스텝1과 마찬가지로 lms에 있는 명세를 그대로 따랐습니다!

컴포넌트 사용법은 리드미 및 스토리북에 남겨놨습니다!

📸 스크린샷 (선택 사항)

2023-09-26.00.45.18.mov

스크린샷에 있는 콘솔이나 테스트 확인 용 상태는 모두 제거되어있습니다.


@gabrielyoon7 gabrielyoon7 changed the title Step2 [고급 레이아웃 컴포넌트] 가브리엘(윤주현) 미션 제출합니다. Sep 25, 2023
@gabrielyoon7 gabrielyoon7 self-assigned this Sep 25, 2023
@gabrielyoon7 gabrielyoon7 requested a review from chsua September 25, 2023 17:59
Copy link

@chsua chsua left a comment

Choose a reason for hiding this comment

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

앞서서 제가 이전 피드백에서 css나 사용성에 대한 질문보다는 빌드, 디자인시스템 등에 관해 많이 물어봤던 것 같아요. 다시 돌아보니 css 관련된 피드백을 하지 않았더라구요 😅 그래서 이번에는 css, 사용성 등에 대해 물어보고자 했습니다.


리뷰

  1. 이해하기 쉬운 코드
  • 전체적으로 코드나 속성이 의미하는 바가 명확해서 좋았습니다. 무슨 역할을 하는지, 어떻게 작동하는 지 이해할 수 있어 코드를 뜯어보지 않아도 쉽게 사용할 수 있을 것 같습니다.
  1. 사용성
  • 주요 기능이 잘 작동하여 좋았습니다.
  • 경우에 따라 해당 바가 눈에 보이는 걸 선호하는 사용자가 있을 것 같아요.
  • default에서 공간에 색상을 넣었는데 가운데 바가 3px 비어있는게 눈에 띄더라구요. 3px로 정한 이유가 있으신가요? 더 작으면 사용자가 클릭해서 움직이기 어려워서 3px로 두신건가요?
  • min/max 를 각각 0%, 100%로 지정하면 의도한 대로 움직이지 않는것 같은데 맞나요?

추가로

해당 pr이나 리드미에는 어느 부분을 고민하여 개발하셨는지 나와있지 않는데 궁금합니다!


체크사항

### 레이아웃 컴포넌트가 여러 환경에서도 잘 보이는지 확인해 주세요

- [x] 반응형: 레이아웃이 다양한 화면 크기에 적응하는지 확인해 주세요.
- [x] 크로스 브라우징: 여러 브라우저(크롬, 사파리, 엣지 등)에서 동작하는지 확인해 주세요.

### 레이아웃 컴포넌트를 사용하는 사용자로서, 잘 이해하고 활용할 수 있었는지 피드백해 주세요

- [x] 가독성: README나 사용법을 보고, 저자의 의도가 잘 이해되는지 독자의 입장에서 확인해 주세요.
- [x] 사용성: 레이아웃 컴포넌트를 사용하는 입장에서 쉽게 사용할 수 있었는지 확인해 주세요.

### 스타일 코드가 네이밍, 재사용성 등을 고려해 잘 작성되어 있는지 확인해 주세요

- [x] 의미론적 네이밍: 해당 CSS 클래스나 ID의 이름이 의미론적으로 적절한지 확인해 주세요.
- [x] 재사용성: 중복되거나 불필요한 스타일 규칙이 있는지 살펴보세요.


export const SplitPaneContainer = styled.div`
display: flex;
flex-direction: row;
Copy link

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.

세로 기능은 고려하지 않았습니다!

export const SplitPaneContainer = styled.div`
display: flex;
flex-direction: row;
height: 100%;
Copy link

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.

제거하였습니당

export const ResizablePane = styled.div<ResizablePaneProps>`
overflow: auto;
min-width: ${({minSize}) => minSize || '0'};
max-width: ${({maxSize}) => maxSize || '100%'};
Copy link

Choose a reason for hiding this comment

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

사용자가 maxSize를 입력하지 않으면 100%가 되는 것 같은데, 이렇게 되면 조절바가 움직이지 않습니다!
예상으로는 조절바가 3px를 차지하고 있어서 그런가 싶기도 한데, 정확하지는 않습니다😂

Copy link
Member Author

Choose a reason for hiding this comment

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

수정하였습니다


export const Resizer = styled.div`
width: 3px;
cursor: col-resize;
Copy link

Choose a reason for hiding this comment

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

오 새로운 속성 배워갑니당

`;

export const Resizer = styled.div`
width: 3px;
Copy link

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.

색상 대신 구분선을 추가했습니다

defaultSize: string;
minSize?: string;
maxSize?: string;
children?: ReactNode[];
Copy link

Choose a reason for hiding this comment

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

children은 왜 otpional props인가요?

Copy link
Member Author

Choose a reason for hiding this comment

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

수정하였습니다

{children[0]}
</ResizablePane>
<Resizer onMouseDown={handleMouseDown}/>
<ResizablePane style={{flex: 1}} minSize={minSize} maxSize={maxSize} ref={rightPaneRef}>
Copy link

Choose a reason for hiding this comment

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

{flex: 1}을 처음 봤는데 실무에서 많이 사용된다고 하네요.
저도 이런 작동이 되도록 고민 많이했는데 해당 코드를 참고하겠습니다👍

Copy link
Member Author

Choose a reason for hiding this comment

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

이 코드는 사라졌습니다 🥲🥲

{children[1]}
</ResizablePane>
</>
) : <div>반드시 2개의 컴포넌트가 존재해야 합니다.</div>}
Copy link

Choose a reason for hiding this comment

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

예외처리 좋아요

@gabrielyoon7
Copy link
Member Author

안녕하세요 수아?

다시 잃어보니 지난번 PR 메세지가 다소 부실했던 것 같네요.
코드를 너무 급하게 작성했었어서 이번에는 많은 부분을 보완했습니다.

SplitPane

<SplitPane
  defaultSize="20%"
  maxSize="90%"
  minSize="10%"
>
  <div>leftPane</div>
  <div>rightPane</div>
</SplitPane>

defaultSize, maxSize, minSize는 좌측 끝 부분으로 부터의 거리를 의미합니다.

image

반드시 % 단위로만 제어가 가능하도록 하였으며, 이는 반응형에서도 일정한 비율로 원활하게 나누기 위함입니다.

따라서 0% ~ 100% 로만 설정이 가능합니다.

코드가 아주 많이 바뀌었는데요, 각종 변경 사항에 대해 말씀드리겠습니다.

중앙 구분선

중앙 구분선이 3px인 이유는, 최소 너비가 있어야 드래그가 가능했기 때문입니다.
현재는 구분선이 눈에 보이도록 설정되었습니다.

오류 메시지 출력

각 프롭의 오류 검사를 진행합니다.
기존에는 자식 엘리먼트가 2개인지만 검사했지만, 지금은 각 프롭마다 유효성 검사를 진행하여 올바른 프롭을 넘길 수 있도록 검사합니다.
(해당 코드는 테스트 코드로도 작성하였으니 참고하시면 좋을 것 같아요)

여담이지만 이 과정에서 jest를 사용하지 않고, 말로만 듣던 vitest를 사용하였는데 소문대로 정말 빠릅니다!

버그 수정

기존에는 0%나 100% 의 경계면에서 다시 잡을 수 없는 문제가 있는 등 여러 문제가 있었습니다.
또, minSize와 maxSize의 합이 100%여야 하는 문제를 수정하였습니다. (이는 제가 컴포넌트에 대한 이해를 잘못 하여 발생했던 것이었습니다.)
프롭 타입도 일부 수정되어 모든 값을 강제로 받도록 개선하였습니다.

Copy link

@chsua chsua left a comment

Choose a reason for hiding this comment

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

늦게 리뷰드려 죄송합니다!
즐거운 연휴 보내고 계신가요~ :)

코드 잘 읽어봤습니다.
에러바운더리에 유효성 검사, 테스트까지 꼼꼼하게 챙기시는게 인상깊습니다.

기존 코드와 많이 달라졌다고 하셨는데, 정말 많이 달라졌더라구요.😁
제가 말씀 드렸던 피드백이 반영된 것이 보여 좋았습니다.

이전에는 min/max가 각 파트별로 적용이 되었던 것 같은데,
지금은 첫번째 파트를 기준으로 min/max가 적용되는 것 같네요!
전체적인 크기가 그대로 유지된다는 점이 좋았습니다 bb

고생하셨습니다!

errorInfo?: ErrorInfo | null;
}

class ErrorBoundary extends Component<Props, State> {
Copy link

Choose a reason for hiding this comment

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

오 아예 에러 바운더리로 처리하셨군요 bb

import {SplitPaneProps} from "./SplitPane.tsx";
import {ResizablePane, Resizer, SplitPaneContainer} from "./SplitPane.styles.ts";

function SplitPaneRenderer({defaultSize, minSize, maxSize, children}: SplitPaneProps) {
Copy link

Choose a reason for hiding this comment

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

renderer로 분리하신 이유가 무엇인가요??

Copy link
Member Author

Choose a reason for hiding this comment

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

에러바운더리를 적용하려면 에러가 하위 컴포넌트에서 발생해야 감지가 됐기 때문입니다...!

export type ValidatePercentageParams = Omit<SplitPaneProps, 'children'>

export const checkEndsWithPercentage = ({defaultSize, minSize, maxSize}: ValidatePercentageParams) => {
if (!defaultSize.endsWith('%') || !minSize.endsWith('%') || !maxSize.endsWith('%')) {
Copy link

Choose a reason for hiding this comment

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

bb

@chsua chsua merged commit 54a79d2 into woowacourse:gabrielyoon7 Oct 2, 2023
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.

2 participants