-
Notifications
You must be signed in to change notification settings - Fork 39
[윤정환] sprint8 #231
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
[윤정환] sprint8 #231
The head ref may contain hidden characters: "React-\uC724\uC815\uD658-sprint8"
Conversation
…nt-Mission into React-윤정환-sprint8
addiescode-sj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다!
궁금해하셨던 Tailwind 사용 관련한 관리 방식, Typescript 사용 관련한 피드백 위주로 리뷰 드려봤어요. 천천히 리팩토링해보세요 :)
주요 리뷰 포인트
- 객체와 함수에 타입 지정 해주기
- TailwindCSS와 CSS Modules 혼용해서 사용하지않기
- TailwindCSS 유틸리티 퍼스트 접근법 컨셉 이해하기
- TailwindCSS 기본 프리셋을 수정하거나 확장하는 방식으로 커스터마이즈하기
- TailwindCSS
@apply사용해 전역적으로 스타일 정의하기 - 함수 시그니처가 동일하지않아 불필요한 타입 구문이 생기는 부분 바로잡기
- 타입 단언 사용을 지양하고, 명확한 타입 사용하기
| export const fieldMap = { | ||
| email: { | ||
| id: "email", | ||
| label: "이메일", | ||
| type: "email", | ||
| placeholder: "이메일을 입력해주세요", | ||
| validator: validateEmail, | ||
| }, | ||
| nickname: { | ||
| id: "nickname", | ||
| label: "닉네임", | ||
| type: "text", | ||
| placeholder: "닉네임을 입력해주세요", | ||
| }, | ||
| password: { | ||
| id: "password", | ||
| label: "비밀번호", | ||
| type: "password", | ||
| placeholder: "비밀번호를 입력해주세요", | ||
| validator: validatePassword, | ||
| }, | ||
| passwordCheck: { | ||
| id: "passwordCheck", | ||
| label: "비밀번호 확인", | ||
| type: "password", | ||
| placeholder: "비밀번호를 다시 입력해주세요", | ||
| validator: validatePasswordCheck, | ||
| }, | ||
| } as const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이런 fieldmap도 재사용 가능성이 있으니 validators 파일 안에서 같이 관리해보는게 좋을것같네요 :)
| const commonFieldProps = { | ||
| formData: loginData.current, | ||
| validationTrigger: isValidating, | ||
| onCheckValidating: setIsValidating, | ||
| errorSetter: setHasError, | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
타입스크립트를 사용하실때는, 객체와 함수에는 꼭 타입 지정을 하시는 게 좋습니다.
이유는, 객체의 속성이나 함수의 매개변수, 반환값에 대한 정보를 실시간으로 확인할수있어 개발 생산성이 크게 향상되기도하고, 함수 시그니처를 변경하거나 객체의 구조를 수정할때도 관련된 모든 코드에서 자동으로 타입 오류가 발생되므로 코드 베이스가 커지더라도 안전하게 작업할 수 있기 때문입니다.
또한, 타입 정의 자체가 문서화 역할을 하기때문에 타입 정의만 보고도 어떤 매개변수를 받아 무엇을 반환하는지, 객체가 어떤 구조를 가지는지 코드만 보고도 이해할수있어 협업에 도움이 됩니다 :)
앞으로 타입스크립트를 사용할때는 이런 점들을 고려해 객체와 함수에는 꼭 타입 정의를 시도해봅시다! :
| className="pt-20 pb-45 px-md | ||
| md:w-160 md:mx-auto md:pt-48 md:px-0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 마이그레이션 도중이라 어쩔수없지만, Tailwind CSS와 CSS Modules를 혼용해서 사용하고 계신데, 이후로는 한가지로 정해서 사용하시는게 좋습니다.
첫번째 이유로는 동일한 속성값에 대해 스타일 우선순위 충돌 문제가 쉽게 발생할수있습니다. 이때 어떤 스타일이 실제로 적용될지는 CSS의 우선순위(CSS Specificity)와 className의 순서에 따라 달라지므로, 혼용해서 사용하게되면 찾기가 어려워지겠죠?
또한, 불필요한 CSS가 번들에 포함되거나, 의도치 않은 스타일이 적용될 수 있습니다.
Tailwind는 자체적으로 PurgeCSS(사용하지 않는 CSS 제거)를 통해 번들 크기를 줄입니다. 이 특성때문에 className이 동적으로 생성되거나, CSS Modules와 조합될 때 Purge가 제대로 동작하지 않을 수 있습니다. CSS Modules는 각 컴포넌트마다 별도의 CSS 파일을 생성하므로, 스타일이 분산되어 관리됩니다.
이 특성을 잘 이해해보시고, 다음 미션때 하나의 방식을 채택해 개선해보시면 좋을것같네요 :)
| "btn primary-btn w-full h-14 rounded-[40px] text-[20px]" | ||
| )} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TailwindCSS는 유틸리티 퍼스트 접근법을 사용하고있습니다.
이 접근법의 특징은 디자인 일관성을 유지하기위해 제한된 스타일 규칙을 사용한다는 점입니다. 따라서 임의값(예: mt-[13px], text-[#123456])을 남용하면, 프로젝트 내에서 디자인 케이스가 많아지고 유틸리티 퍼스트 접근을 따르는 라이브러리를 사용하는 의미가 퇴색되겠죠? :)
만약 자주 사용하는값이 기본 프리셋에 없다면,
아래와 같이 tailwind config 파일을 수정하는 방법으로 value를 커스터마이즈하거나 확장하여 임의값 사용을 줄여보시는 게 좋습니다.
// tailwind.config.ts
module.exports = {
theme: {
extend: {
borderRadius: {
'btn': '40px', // rounded-btn
},
fontSize: {
'btn': '20px', // text-btn
},
},
},
plugins: [],
}<button className="btn primary-btn w-full h-btn rounded-btn text-btn">
버튼 텍스트
</button>There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
혹은, Tailwind의 @apply를 활용해 전역적으로 버튼 스타일을 정의할 수도 있습니다.
- globals.css
/* src/styles/global.css */
.btn {
@apply w-full h-btn rounded-btn text-btn font-bold;
}
.primary-btn {
@apply bg-blue-500 text-white hover:bg-blue-600;
}| label: string; | ||
| type: "email" | "text" | "password"; | ||
| placeholder: string; | ||
| validator?: SingleParamValidator | TwoParamValidator; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
타입을 이렇게 사용하면, 타입이 SingleParamValidator이거나 TwoParamValidator인 함수를 받게 되는데, 두 함수의 시그니처가 다르기때문에 실제 사용할때는 타입 단언을 하거나, 타입 가드를 사용해 어떤 시그니처로 호출할지 명확히해야하는 특징이 있습니다.
if (validator && id === "passwordCheck") {
// TwoParamValidator로 사용
validator(value, comparisonValue);
} else if (validator) {
// SingleParamValidator로 사용
(validator as SingleParamValidator)(value);
}만약 항상 동일한 시그니처를 사용하고싶다면, 타입을 아래와 같이 써주는게 좀 더 안전하고, 불필요한 타입 구문을 효과적으로 제거할 수 있는 방법이 되겠죠?
type Validator = (value: string, comparisonValue?: string) => { isValid: boolean; errorMsg: string | null };| if (id in formData && inputRef.current) { | ||
| formData[id as keyof ILoginData] = inputRef.current?.value; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
타입스크립트가 id가 실제로 ILoginData의 key임을 확신할 수 없기 때문에 타입 단언을 쓴 것같은데, 이런 방식은 오히려 id가 실제로 ILoginData의 key가 아닐 수도 있는데 타입 단언으로 강제로 우회하고 있는 형태라서 타입 안전성을 해치고, 런타임 에러의 가능성을 높일 수 있어요 :)
Props에서 id의 타입을 keyof ILoginData로 지정하면, 타입 단언이 필요 없어지겠죠?
interface Props {
id: keyof ILoginData;
// ...생략
}타입 단언을 사용하게되는 순간 타입스크립트의 타입 시스템을 우회하므로,
가능하다면 타입 단언을 사용하지않고 타입을 명확히 제한하는 편이 좋습니다 :)
| if (validator && id === "passwordCheck") { | ||
| const { isValid, errorMsg } = validator(value, comparisonValue); | ||
|
|
||
| if (!isValid) { | ||
| errorSetter(true); | ||
| setErrorMsg(errorMsg); | ||
| onCheckValidating(false); | ||
| return; | ||
| } | ||
| } else if (validator) { | ||
| const { isValid, errorMsg } = (validator as SingleParamValidator)(value); | ||
|
|
||
| if (!isValid) { | ||
| errorSetter(true); | ||
| setErrorMsg(errorMsg); | ||
| onCheckValidating(false); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 분기는 위에서 언급한것처럼 validator의 타입을 동일한 함수 시그니처로 사용할수있도록 맞춰주면 필요없어지겠네요 :)
질문에 대한 답변
일반적으로 공통 로직이 3군데 이상 반복된다면, 훅/컴포넌트 등으로 추출을 권장하는 편입니다. 만약 조금이라도 다르거나, 앞으로 더 달라질 가능성이 있다면 중복을 허용하는 것이 오히려 관리가 쉬워질거예요 :) |
요구사항
배포
https://sprint-mission8.netlify.app/
기본
심화
주요 변경사항
스크린샷
멘토에게