[Feat] TextField 컴포넌트 제작#21
Hidden character warning
Conversation
PR 검증 결과✅ TypeScript: 통과 |
PR 검증 결과✅ TypeScript: 통과 |
KyeongJooni
left a comment
There was a problem hiding this comment.
고생하셨습니다! :) 코멘트 남겼으니까 확인해주시면 좋을 것 같아요 👍
| // 표시값(1,000) -> 숫자만(1000) | ||
| next = next.replace(/\D/g, ''); | ||
| // consumer가 e.target.value 읽는 경우를 위해 숫자로 맞춰줌 |
There was a problem hiding this comment.
주석을 정리해서 달아주시거나 불필요한 주석은 삭제해도 좋을 것 같아요!
| <textarea | ||
| ref={textareaRef} | ||
| value={digitsValue} |
There was a problem hiding this comment.
digitsValue 라는 변수명이 살짝 모호한거 같아요
const normalizedValue = isPrice ? rawValue.replace(/\D/g, '') : rawValue;이런식으로 수정하셔도 좋을 것 같습니다!
| const styles = textFieldVariants({ | ||
| error, | ||
| disabled, | ||
| filled, | ||
| price: isPrice, | ||
| }); |
There was a problem hiding this comment.
현재 variant에 적용되어있는 char는 정의하고 사용은 안하는 걸까요?
There was a problem hiding this comment.
다시 확인해 보니까 현재 UI에서는 counter가 header 영역에 있어 char variant가 사용되지 않네요...!! 필요하지 않을 것 같아 제거하였습니다!
| 'resize-none', | ||
| 'text-[16px] leading-[24px] font-medium', | ||
| 'placeholder:text-[var(--color-gray-300)]', | ||
| 'hover:border-[var(--color-green-700)]', | ||
| 'focus:border-[var(--color-green-700)]', | ||
| 'focus:outline-none', | ||
| 'transition-colors', | ||
| 'resize-none', |
There was a problem hiding this comment.
중복 정의하신 이유가 있을까요? 없다면 삭제하는게 좋을 것 같습니다!
There was a problem hiding this comment.
리뷰해 주신 대로 input / textarea에 중복되던 스타일을 fieldBase로 공통화하고, resize-none, text / placeholder / borer / transition 등은 중복 정의 제거하였습니다!
| <div className={styles.labelRow()}> | ||
| {label && <label className={styles.label()}>{label}</label>} | ||
| {showCounterInHeader && ( | ||
| <span className={styles.counter()}> | ||
| {currentLength}/{maxLen} | ||
| </span> | ||
| )} | ||
| </div> |
There was a problem hiding this comment.
char를 정의하고 사용하려고 해도 char가 counter를 absolute로 만드는 속성을 가진 것 같은데 fieldWrapper랑 counter 가 분리가 되어 있는거 같아서 혹시 작동하는 확인 부탁드립니다! 😄
There was a problem hiding this comment.
위의 답변과 마찬가지로 char variant를 제거하였습니다!
| if (!isControlled) { | ||
| setUncontrolledValue(next); | ||
| } | ||
| onChange?.(e as React.ChangeEvent<HTMLInputElement>); |
There was a problem hiding this comment.
이미 타입을 정의할 때 유니온을 받아서 assertion이 필요가 없을 것 같습니다!
| onChange?.(e as React.ChangeEvent<HTMLInputElement>); | |
| onChange?.(e); |
| @@ -0,0 +1,149 @@ | |||
| import { textFieldVariants } from '@shared/ui/TextField/TextField.variants'; | |||
| import { useLayoutEffect, useMemo, useRef, useState } from 'react'; | |||
| import type { TextFieldProps } from '@shared/ui/TextField/TextField.types'; | |||
There was a problem hiding this comment.
TextFieldType 은 정의하고 사용 안하고 있는 것 같습니다! 확인해보시면 좋을 것 같아요
There was a problem hiding this comment.
TextFieldType은 TextFieldProps의 type에 사용 중이라 내부 정의만 유지하고, 외부 사용처가 없어 export만 제거했습니다!
| (ControlledTextFieldProps | UncontrolledTextFieldProps) & | ||
| Omit<ComponentPropsWithoutRef<'input'>, 'type' | 'value' | 'defaultValue' | 'onChange'> & | ||
| Omit<ComponentPropsWithoutRef<'textarea'>, 'value' | 'defaultValue' | 'onChange'> & { | ||
| onChange?: (e: CommonChangeEvent) => void; |
There was a problem hiding this comment.
여기도 ChangeEventHandler 사용해서 타입 정의하는건 어떨까요?
| export { TextField } from './TextField'; | ||
| export type { TextFieldProps } from './TextField.types'; |
There was a problem hiding this comment.
타입 임포트가 누락된거 가타요!
| export { TextField } from './TextField'; | |
| export type { TextFieldProps } from './TextField.types'; | |
| export { TextField } from './TextField'; | |
| export type { TextFieldProps, TextFieldType } from './TextField.types'; |
There was a problem hiding this comment.
현재 모든 텍스트 필드를 한 파일이 담당하고 있어 덩어리가 큰 것 같습니다 기본 래퍼는 거의 동일해서
TextField/
├── TextField.tsx # 공통 로직
├── CharField.tsx # type="char" 전용
├── TextAreaField.tsx # type="textarea" 전용
├── TextField.types.ts # 타입정의
├── TextField.variants.ts #베리언트 파일
├── PriceField.tsx # type="price" 전용
└── index.ts # 통합 export
이런식으로도 분리 고려해볼 수도 있을 것 같습니다!
There was a problem hiding this comment.
말씀해 주신 구조로 CharField / TextAreaField / PriceField 래퍼를 분리해서 적용했습니다! 공통 로직은 TextField.tsx에 유지했습니다 추후에 전용 로직이 커지면 더 세분화하겠습니다!
|
예빈님 textfield 구현 힘드실텐데 수고하셨습니다! |
PR 검증 결과✅ TypeScript: 통과 |
PR 검증 결과✅ TypeScript: 통과 |
PR 검증 결과✅ TypeScript: 통과 |
KyeongJooni
left a comment
There was a problem hiding this comment.
수정사항 반영된거 확인했고 추가로 코멘트 좀 더 달았습니다 확인부탁드려요! :)
| type="text" | ||
| value={displayValue} | ||
| disabled={disabled} | ||
| maxLength={maxLen} |
There was a problem hiding this comment.
maxLen이 콤마도 포함해서 displayValue에 적용될 것 같은데
<input
maxLength={isPrice ? undefined : maxLen}
// ...
/>이런식으로 수정하시면 더 좋을 것 같습니다!
아니면 handleChange에서 숫자 길이 제한하는 방법도 있을 것 같습니다!
There was a problem hiding this comment.
말씀해 주신 대로 price 입력은 콤마 포함 길이로 제한되지 않도록 숫자만 추출한 값에 maxLength를 적용하도록 수정했습니다!
| const maxLen = isChar | ||
| ? CHAR_MAX_LENGTH | ||
| : (maxLength ?? (isTextarea ? TEXTAREA_MAX_LENGTH : undefined)); |
There was a problem hiding this comment.
중첩 삼항 연산자때문에 가독성이 쪼금 떨어지는 것 같아요 객체매핑을 통해 정의하거나 함수로 분리 고려해보시면 좋을 것 같습니다!
There was a problem hiding this comment.
중첩 삼항 대신 getMaxLength 함수로 분리했습니다!
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR 검증 결과✅ TypeScript: 통과 |
PR 검증 결과✅ TypeScript: 통과 |
PR 검증 결과✅ TypeScript: 통과 |
✨ 주요 변경사항
📝 작업 상세 내용
✅ 체크리스트
Close #번호추가📸 스크린샷 (선택)
🔍 기타 참고사항
원suffix와의 간격은 임의로 30픽셀로 지정해 놓았습니다!🔗 관련 이슈