경북대 FE_이정민_1주차 과제 Step2,3 #76
Conversation
There was a problem hiding this comment.
말씀해주신 것처럼 스타일이 제공된 것과 달라서 스타일까지 세세하게 리뷰를 남기지는 않았어요.
- className을 작성하실때 storybook이라는 prefix를 사용하시는 이유가 있을까요?
- css 파일을 styles 폴더에 몰아 넣을 경우, 개발하실때 불편하지는 않았나요?
- 컴포넌트.tsx 파일과 스토리북 파일들이 모두 같은 deps에 flat하게 펼쳐저 있는데, 혹시 헷갈리거나 복잡해 보인다는 느낌을 받지는 않았나요?
그리고 과제에서 css module대신 별도의 스타일 라이브러리를 사용하도록 안내하지 않았나요...?
요것은 확인해보시면 좋을 것 같아요!
There was a problem hiding this comment.
stories 폴더 전체가 불필요하므로 모두 제거되어도 좋을 것 같습니다~
| isRanking?: boolean; | ||
| rankingIndex?: number; |
There was a problem hiding this comment.
랭킹 여부를 isRanking로 따로 받을 필요가 있을까요?
rankingIndex의 여부로 isRanking을 대체하면 어떨까요?
| let themeType = ''; | ||
| switch (theme) { | ||
| case 'Con': | ||
| themeType = 'storybook-button--con'; | ||
| break; | ||
| case 'Muji': | ||
| themeType = 'storybook-button--muji'; | ||
| break; | ||
| case 'JayG': | ||
| themeType = 'storybook-button--jayg'; | ||
| break; | ||
| default: | ||
| themeType = ''; | ||
| } |
There was a problem hiding this comment.
| let themeType = ''; | |
| switch (theme) { | |
| case 'Con': | |
| themeType = 'storybook-button--con'; | |
| break; | |
| case 'Muji': | |
| themeType = 'storybook-button--muji'; | |
| break; | |
| case 'JayG': | |
| themeType = 'storybook-button--jayg'; | |
| break; | |
| default: | |
| themeType = ''; | |
| } | |
| const styleByTheme = { | |
| 'Con': 'storybook-button--con', | |
| 'Muji': 'storybook-button--muji', | |
| 'JayG': 'storybook-button--jayg', | |
| } |
let으로 선언된 변수는 재할당이 가능해서 이후 어디서 어떻게 변하는지 추적하기 어려울 수 있어 가능하면 지양하는 편이에요.
해당 조건부 스타일 적용은 객체를 사용하면 더 짧게 표현할 수 있을 것 같아요!
물론 스타일을 적용하는 부분도 조금 수정이 필요하겠죠?
| } | ||
|
|
||
| export const Button = ({ | ||
| primary = false, |
There was a problem hiding this comment.
생각해보면 primary라는 프로퍼티는 theme의 일부라고 볼 수 있을 것 같아요.
만약 둘이 다른 속성으로 동작하려면 각 theme별로 primary 스타일이 지정되어 있어야 하지 않을까 싶어요.
또한, 일반적으로 primary는 boolean으로 핸들링하지 않고, 'primary' | 'secondary' 처럼 버튼의 속성을 유니온 타입으로 관리합니다.
| if (radius === 'circle') { | ||
| style.borderRadius = '50%'; | ||
| } else if (typeof radius === 'string' && !Number.isNaN(Number(radius))) { | ||
| style.borderRadius = `${Number(radius)}px`; | ||
| } else if (typeof radius === 'number') { | ||
| style.borderRadius = `${radius}px`; | ||
| } |
| @@ -0,0 +1,49 @@ | |||
| import '@styles/button.css'; | |||
|
|
|||
| interface ButtonProps { | |||
There was a problem hiding this comment.
button tag의 모든 prop을 받을 수 있도록 extends로 타입을 확장해 주세요.
그 과정에서 아래에 작성해주신 prop 중 button이 원래 갖고있는 prop과 겹치는게 나올 수 있을텐데요, 타입스크립트의 유틸리티 타입 중에서 Omit이 도움이 될 거에요.
| interface InputProps { | ||
| type?: 'text' | 'number'; | ||
| placeholder?: string; | ||
| value?: string; | ||
| onChange?: (e: React.ChangeEvent<HTMLInputElement>) => void; | ||
| size?: 'small' | 'medium' | 'large' | 'responsive'; | ||
| disabled?: boolean; | ||
| invalid?: boolean; | ||
| } |
There was a problem hiding this comment.
input 역시 extends로 타입을 확장해주시면 좋을 것 같아요~!
| - [x] Theme Props에 따라 버튼의 컬러와 디자인이 변경되도록 구현 | ||
| - [x] Size Props에 따라 버튼의 size가 변경되도록 구현 | ||
| - [x] Size Props 예외 - value가 responsive인 경우 미디어 쿼리에 따라 사이즈가 변경되도록 구현 | ||
| - [x] Button element의 기본 속성을 모두 사용 가능하도록 구현 |
| - [x] invalid Props 에 따라 Input의 값이 잘 못되었음을 UI에서 인지 가능하도록 구현 | ||
| - [x] Size Props에 따라 버튼의 size가 변경되도록 구현 | ||
| - [x] Size Props 예외 - value가 responsive인 경우 미디어 쿼리에 따라 사이즈가 변경되도록 구현 | ||
| - [x] Input Element의 기본 속성들을 모두 사용 가능하도록 구현 |
왜 하나로 묶어주면 성능이 향상되나요?
바닐라로 변환 후에 어떤 모습 띄나요?
컴포넌트의 Props와 state를 통해 무엇을 관리하나요? |
안녕하세요 멘토님,
경북대학교 FE 이정민입니다.
[Step2]
엉성한 코드가 많지만 최대한 기능 목록을 위주로 구현해보았습니다.
제출 기한이 임박하여 Container의 Full screen 옵션 등 참고 Storybook과 세부사항이 동일하지 않은 곳이 있습니다.
해당 부분은 코드 리뷰와 함께 다음 주차에 수정해서 업데이트 하도록 하겠습니다.
소중한 시간 내어 코드 리뷰 진행해주셔서 감사합니다.
[Step3]
webpack은 무엇이고 어떤 역할을 하고 있나요?
: 웹팩은 모듈 번들러로서 개발할 때 사용되는 파일들을 모듈 단위로 관리하고 하나의 bundle로 묶어주는 역할을 합니다. 따라서 하나로 묶어주어 성능이 향상되는 효과가 있습니다.
브라우저는 어떻게 JSX 파일을 읽을 수 있나요?
: 브라우저는 jsx 파일을 직접 읽을 수 없고, 브라우저가 이해할 수 있는 vinilla js로 코드를 변환하는 과정이 필요합니다. 이때 Babel을 주로 사용합니다.
React에서 상태 변화가 생겼을 때 어떻게 변화를 알아챌 수 있나요?
: 컴포넌트의 Props와 state를 통해 관리합니다. 상태 변화가 생기면 리액트는 setState로 감지하고 컴포넌트를 다시 렌더링합니다.