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

[기본 레이아웃 컴포넌트] 윤생(이윤성) 미션 제출합니다. #11

Merged
merged 26 commits into from
Sep 23, 2023

Conversation

2yunseong
Copy link

@2yunseong 2yunseong commented Sep 18, 2023

안녕하세요 가람! 이번에 서로 리뷰어, 리뷰이가 되었네요!! 잘 부탁드립니다 :)

부족한 부분이 많은데 가람께서 생각하는 부분 자유롭게 지적해주시기 바랍니다!!

감사합니다!!

🎯 1단계. 기본 레이아웃 컴포넌트

npm 배포 주소 : https://www.npmjs.com/package/yunseong-layout-component
MUI 처럼 실제 동작을 확인할 수 있는 문서는 아직 구현하지 못했습니다..!
스토리북 배포 주소 : https://650940268ad0c856c40a323c-lmmjnzzviz.chromatic.com/

공통

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

Container 컴포넌트

  • minWidth, maxWidth 속성 적용
  • npm 배포
  • README.md 작성
  • 테스트 코드를 작성하였나요? (선택 사항)

Grid 컴포넌트

  • rows, columns, gap 속성 적용
  • npm 배포
  • README.md 작성
  • 테스트 코드를 작성하였나요? (선택 사항)

Flex 컴포넌트

  • direction, justify, align, gap 속성 적용
  • npm 배포
  • README.md 작성
  • 테스트 코드를 작성하였나요? (선택 사항)

📌 구현한 내용 설명

  • 기존요구사항에, grid에 속하는 item이 공간을 차지할 수 있는 grid-column 속성과 grid-rows 속성을 추가하였습니다.
  • 사용자가 어느 부분까지 속성을 건드릴 수 있어야 되는지 고민이 많이 되었습니다. 예시로 flex의 모든 속성을 변경할 수 있도록 하면, 그냥 css를 사용하는 것과 다름이 없고, 또 속성을 너무 제한한다면 사용하는 개발자가 원하는 바를 이루지 못할 것 같아, 편의성과 확장성이 상충되는 것 같아요. 가람은 어떻게 생각하시나요?
  • 추가로 css 같은 경우는 잘못된 속성을 주면 브라우저가 무시해버리는데요, (ex : grid-column 속성에는 그리드의 열을 나타내는 자연수만 올 수 있지만 실수나 다른 값이 오면 무시해버림) 이걸 타입스크립트에서 제한하는 것에 대해 어떻게 생각하시나요??

Copy link

@guridaek guridaek left a comment

Choose a reason for hiding this comment

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

안녕하세요 윤생!
리뷰가 늦어져서 죄송합니다!!! 🙇‍♂️🙇‍♂️🙇‍♂️
다음 요청 주시면 바로 머지하겠습니다!

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

크롬, 사파리 둘 다 잘 동작하는 것 같아요.
다만, 콘솔에 에러가 나오는 것 같네요. 🥲
image

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

리드미를 자세하게 작성하신 것 같아요.
flex, grid를 사용해본 사람이라면, 이해가 어렵지 않을 것 같아요.

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

이 부분은 무난하게 작성하신 것 같아요.
기존 속성들의 이름을 그대로 쓰거나, 간단히 축약한 정도라 이해가 쉬운 것 같습니다.


윤생이 질문주신 부분인 '어디까지 속성을 제한해야 하는가'는 저도 많이 고민됐던 부분입니다.
재 생각은, 컴포넌트들의 테마를 확실하게 맞춰서 '내 라이브러리다!' 라는 느낌을 주려면 제한을 많이 둬야 할 것 같고, 그게 아니고 공통된 스타일이 적은 컴포넌트들이면 자유도를 높게 줄 것 같습니다.

props를 타입스크립트로 제한을 두는 건 취향 차이일 것 같긴 한데..
윤생이 지금 한 것 처럼 리드미에만 적어두는 정도가 좋지 않을까 생각합니다 ㅎㅎ

.babelrc.json Show resolved Hide resolved
dist/cjs/components/Container/index.js Show resolved Hide resolved
package.json Show resolved Hide resolved
src/components/Container/index.tsx Outdated Show resolved Hide resolved
src/components/Grid/index.tsx Show resolved Hide resolved
src/stories/Flex.mdx Show resolved Hide resolved
src/components/FlexItem/index.tsx Show resolved Hide resolved
@2yunseong
Copy link
Author

가람 안녕하세요! 윤생입니다!!
짚어주신 에러 부분은 에러 메세지에서 알려준대로 lowercase로 해결하였습니다~!
리뷰 재 요청 드려요! 주말 잘 보내세요!! 🫡

Copy link

@guridaek guridaek left a comment

Choose a reason for hiding this comment

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

안녕하세요 윤생!
수정하신 부분 확인했습니다!
데모데이 준비, 미션 같이 하시느라 고생 많으셨습니다! 👏
2단계도 화이팅입니다!

@guridaek guridaek merged commit d9f6b6b into woowacourse:2yunseong Sep 23, 2023
@2yunseong 2yunseong deleted the 2yunseong branch September 24, 2023 08:04
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