Skip to content

[1단계 - 페이먼츠] 소하(최소연) 미션 제출합니다.#344

Merged
JeongBin0227 merged 57 commits into
woowacourse:soi-hafrom
soi-ha:step1
Apr 22, 2024
Merged

[1단계 - 페이먼츠] 소하(최소연) 미션 제출합니다.#344
JeongBin0227 merged 57 commits into
woowacourse:soi-hafrom
soi-ha:step1

Conversation

@soi-ha
Copy link
Copy Markdown

@soi-ha soi-ha commented Apr 18, 2024

안녕하세요, 케빈!
Level2의 첫번째 리뷰네요..! 아직 많이 부족하지만.. 잘 부탁드립니다. :)

실행 방법

// 로컬 서버 실행
npm run dev

// 스토리북 실행
npm run storybook

기능 구현 확인 링크

🔗 페이먼츠 배포 링크

📕 페이먼츠 컴포넌트 스토리북 링크

컴포넌트 구조 이미지

소하, 페이먼츠 컴포넌트 구조 이미지

리뷰 전, 참고 사항

  • React, TypeScript, Styled-components를 사용했습니다.
  • React는 현재 적용한 수준정도만 알고 있습니다!
  • TypeScript는 Level1에서 처음 해봤어요.
  • styled-components는 조금 해봤습니다. (현재 적용한 수준 정도만 알아요!)

기능 요구 사항

  • 카드 번호 입력 및 식별
    사용자가 입력하는 카드 번호를 실시간으로 파악하여, Visa나 MasterCard에 해당하면 해당 브랜드의 로고를 UI에 표시한다.
    입력은 숫자만 가능하며, 유효하지 않은 번호 입력 시 피드백을 제공한다.
  • 카드 유효기간 입력
    월과 년도를 범위 내에서만 입력할 수 있어야 하며, 입력 제한을 두어 사용자가 숫자만 입력할 수 있도록 한다.
  • 카드 소유자 이름 입력
    영문자 대문자만 입력 가능한 폼을 구현한다.
  • 실시간 프리뷰 업데이트
    사용자의 카드 정보 입력에 따라 카드 프리뷰를 동시에 업데이트한다.

주요 기능 구현 사항

자세한 기능 요구사항은 README.md를 참고해주세요.

  • 카드번호
    • 숫자 외의 값을 입력하면, 에러 메세지가 출력되고 border 색상은 red가 된다.
    • 입력한 숫자가 모두 16자리인지 확인한다.
  • 유효기간
    • 숫자 외의 값을 입력하면, 에러 메세지가 출력되고 border 색상은 red가 된다.
    • 입력한 숫자가 유효한 기간인지 확인하고, 유효하지 않다면 에러 메세지 출력 및 border 색상을 red로 변경시킨다.
  • 소유자 이름
    • 영어 외의 값을 입력하면, 에러 메세지가 출력되고 border 색상은 red가 된다.
    • 소문자로 작성시, 자동으로 대문자로 변경시킨다.
  • 카드 프리뷰
    • 입력사항을 실시간으로 반영한다.
    • 카드번호가 4로 시작하는 16자리 숫자인 경우, Visa 로고를 출력한다.
    • 카드번호가 51~55로 시작하는 16자리 숫자인 경우, MasterCard 로고를 출력한다.
    • 소유자 이름이 특정 넓이를 넘어갈 경우 말줄임표(...)로 축약한다.

질문 사항

질문 사항을 작성하면서, 제가 작성한 질문 목록은 개발자의 성향에 따라 달라질 것 같다는 생각이 들었습니다.
그래서 해당 질문에 대한 답변은 ‘케빈’은 어떻게 하는지 알고 싶습니다!

  • styled-components는 사용하는 파일 내부에 정의하는게 좋은가?

    이전 Level1의 경우에는 css 파일이 따로 존재하여, 각 컴포넌트마다 css 파일을 생성하여 스타일을 정의하였습니다.

    이번에 Level2가 되면서 styled-components를 사용하게 되었는데요. styled-components를 사용한 예시 코드들을 보면 컴포넌트 파일 내부에 스타일을 정의하고 있었습니다. 따로 파일을 분리하지 않고요.

    styled-components는 사용하는 컴포넌트 파일 내부에 정의하는 것이 더 선호될까요?

    사실 질문을 작성하면서 든 생각은 이름 그대로 components니까, 해당 파일에 작성해주는 것이 더 옳을 것 같다는 생각이 들긴 했습니다!

  • 스타일 코드가 길어지게 된 경우, 파일을 분리하는 것이 좋은가?

    styled-components를 사용했고, 컴포넌트 파일에 스타일을 정의한다고 가정하겠습니다.

    스타일 코드가 길어지게 되면 하나의 컴포넌트 파일은 코드가 길어지게 될 것입니다. (스타일 코드 + 기능 코드로 인해)
    하나의 파일에서 모두 확인할 수 있다는 점은 좋겠지만, 파일의 코드가 길어지니 가독성이 좋지 않을 것이라 생각이 들었습니다.

    이런 경우에는 스타일 코드를 다른 파일로 분리하는 것이 좋을까요?

  • 자주 사용하는 스타일 컴포넌트는 외부 파일로 분리하여 관리하는 것이 좋은가?

    여러 곳에서 사용하는 스타일 컴포넌트의 경우에는 외부 파일로 따로 분리하여 관리하는 것이 좋을까요?
    아니면, 처음 사용하는 곳에서 정의하고 export 하여 재사용하는 곳에서 import 하는 것이 좋을까요?

Todari and others added 30 commits April 16, 2024 16:39
Co-authored-by: soi-ha<soy2302ten@gmail.com>
Co-Authored-By: TaehunLee <85233397+todari@users.noreply.github.com>
Co-Authored-By: TaehunLee <85233397+todari@users.noreply.github.com>
Co-Authored-By: TaehunLee <85233397+todari@users.noreply.github.com>
Co-Authored-By: TaehunLee <85233397+todari@users.noreply.github.com>
Co-Authored-By: TaehunLee <85233397+todari@users.noreply.github.com>
Co-Authored-By: TaehunLee <85233397+todari@users.noreply.github.com>
Co-Authored-By: TaehunLee <85233397+todari@users.noreply.github.com>
Co-Authored-By: TaehunLee <85233397+todari@users.noreply.github.com>
Co-Authored-By: TaehunLee <85233397+todari@users.noreply.github.com>
…om Hook 생성

Co-Authored-by: soi-ha <soy2302ten@gmail.com>
Co-authored-by: Soyeon Choe <soy2302ten@gmail.com>
Co-Authored-by: soi-ha <soy2302ten@gmail.com>
Co-Authored-by: soi-ha <soy2302ten@gmail.com>
Co-Authored-by: soi-ha <soy2302ten@gmail.com>
Co-Authored-by: soi-ha <soy2302ten@gmail.com>
Co-Authored-by: soi-ha <soy2302ten@gmail.com>
Co-Authored-by: soi-ha <soy2302ten@gmail.com>
Co-Authored-by: soi-ha <soy2302ten@gmail.com>
Co-Authored-by: soi-ha <soy2302ten@gmail.com>
Co-Authored-by: soi-ha <soy2302ten@gmail.com>
Co-Authored-by: soi-ha <soy2302ten@gmail.com>
Co-Authored-by: soi-ha <soy2302ten@gmail.com>
Co-Authored-by: soi-ha <soy2302ten@gmail.com>
soi-ha and others added 4 commits April 18, 2024 16:16
Co-Authored-By: TaehunLee <85233397+todari@users.noreply.github.com>
Co-Authored-By: TaehunLee <85233397+todari@users.noreply.github.com>
Co-Authored-By: TaehunLee <85233397+todari@users.noreply.github.com>
@JeongBin0227 JeongBin0227 self-assigned this Apr 18, 2024
@JeongBin0227
Copy link
Copy Markdown

styled component 관련한 태그에 대한 답변을 드리면,

코멘트에도 남겼지만 가장 중요한건 통일성이라고 생각합니다.

분리하던지 파일내부에 작성되던지 통일되어야 한다고 생각합니다.

여기에 장단점이 있는데, 분리가 되면 파일 길이가 짧아지고 가독성이 좋다고 생각합니다. 지금 소하의 코드는 전체적으로 파일의 코드라인이 길다고 생각합니다. 그런 이유로 나누면 더 좋을거 같아요. 또한 나누게 되면 스타일 태그와 컴포넌트가 확실히 분리되어 더 좋은 측면도 있습니다.
파일명.styled.ts 이런식으로 하면 되겠네요.

물론 파일안에 적어도 장점이 있습니다. 해당 파일을 여러가지 안봐도 한눈에 어떤 스타일인지 파악이 가능합니다. 여기서 중요한건 파일내부에 작성하게 된다면 파일 제일 하단에 작성되는게 좋다고 생각합니다. 왜냐하면 스타일은 로직보다 우선순위가 낮다고 생각하기 때문입니다.

@JeongBin0227
Copy link
Copy Markdown

배포해주신 결과물을 봤는데, 버그도 없고 사용자 경험을 고려 많이해주신거 같아서 너무 좋았습니다 👍

Copy link
Copy Markdown

@JeongBin0227 JeongBin0227 left a comment

Choose a reason for hiding this comment

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

안녕하세요 소하! 만나서 반갑습니다

질문 주신 부분은 코멘트로 남겨드렸으니 확인해주시면 될거같아요. 전체적으로 사용자 경험을 많이 고려해주셔서 좋았습니다. 👍
코드적으로 아쉬운게 있다면 구조는 잘 나눠져있는데 파일당 코드 라인수가 너무 많아 가독성이 조금 떨어졌습니다. 지금은 괜찮지만 좀더 확장된다면 유지보수가 힘들지 않을까 라는 생각도 들었습니다. 이부분 유의하면서 리팩토링을 진행해봐도 좋을거 같아요!

페이먼츠 미션하느라 고생많으셨습니다 👍 💯

Comment thread src/components/CardBrand.tsx Outdated
changBrand();
}, [firstCardNumbers]);

return <>{isBrand ? <Image src={BRAND_TABLE[brand]} /> : <></>}</>;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

<></> 대신 null 은 어떨까요?

Comment thread src/components/CardBrand.tsx Outdated
const [brand, setBrand] = useState<CardBrand>('Visa');
const [isBrand, setIsBrand] = useState(false);

const changBrand = () => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

changBrand 에 오타가 있는거 같네요~

이런 에러들은 주의한다고 해서 쉽게 고칠수 있는게 아니여서
Code Spell Checker 같은 VSC 익스텐션 사용을 추천 드려요~~

Comment thread src/components/CardBrand.tsx Outdated
Comment on lines +8 to +11
const Image = styled.img`
width: 36px;
height: 28px;
`;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

이런 스타일 태그를 작성� 규칙은 없지만,

개인적으로 제일 중요한건 통일성이라고 생각합니다.
분리하던지 파일내부에 작성되던지 통일되어야하고, 파일내부에 작성하게 된다면 파일 제일 하단에 작성되는게 좋다고 생각합니다.
왜냐하면 스타일은 로직보다 우선순위가 낮다고 생각하기 때문입니다.

Comment thread src/components/CardBrand.tsx Outdated
width: 36px;
height: 28px;
`;
type CardBrand = 'Visa' | 'MasterCard';
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

보통 타입명을 작성할때는 명확하게 작성해주는게 좋아요.
여기서는 CardBrandType 이 좋겠네요. 근데 해당 타입은 export 되어 다른곳에 사용되지 않으니 그냥 Props 나 Type 으로 정해도 될거 같아요.

setBrand('Visa');
setIsBrand(true);
} else if (REGEX.startsWith5155.test(firstCardNumbers)) {
setBrand('MasterCard');
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

MasterCard 이런 값들은 상수로 따로 관리해줘도 좋겠네요

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

상수화를 해주면 타입이 단순 string이 되어버려 Props의 타입과 일치하지 않아 타입에러가 발생했습니다..!
그래서 VisaMasterCard를 상수화 시킨 것을 다시 돌려뒀습니다..

Comment on lines +20 to +31
const initializeInputFieldState = (length: number) => {
const obj: InputStates = {};
for (let i = 0; i < length; i++) {
obj[i] = {
value: '',
hasError: false,
hasFocus: i === 0,
isFilled: false,
};
}
return obj;
};
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

initializeInputFieldState 함수 내에서 객체를 만들 때 반복문 대신 Array.from과 reduce를 사용하여 초기 상태를 만드는 것이 한 번에 처리하는 측면에서 더 깔끔하고 효율적일거 같아요.
예를 들면

const initializeInputFieldState = (length: number) => {
  return Array.from({ length }, (_, index) => ({
    value: '',
    hasError: false,
    hasFocus: index === 0,
    isFilled: false,
  })).reduce((acc, curr, index) => {
    acc[index] = curr;
    return acc;
  }, {} as InputStates);
};

이렇게요!

<InputForm>
<Label>카드번호</Label>
<InputFieldContainer className="input-field-container">
{[...Array(OPTION.cardNumberInputCount)].map((_, index) => (
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

바로 Array.from 을 사용해도 될거 같아요

&:focus {
outline: none;
border: ${(props) =>
props.hasError ? 'solid 1px #ff3d3d' : 'solid 1px #000000'};
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

색상값들이 ff3d3d 이런게 되어있으면 알기 힘드니 다 변수로 관리해서 바로 어떤색인지 알수있게 하면 가독성이 올라갈거 같아요

}
};

const handleOnFocus = (index: number) => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

handleOnFocus, handleOnBlur와 같은 함수명은 일반적으로 이벤트 핸들러에서 사용하는 네이밍 규칙입니다. 해당 함수들은 상태를 업데이트하는 행위이므로, setFocus와 clearFocus와 같이 동작을 반영하는 이름으로 변경하는 것은 어떨까요?

Comment on lines +18 to +37
const nowDate = new Date();
const year = nowDate.getFullYear().toString().slice(2, 4);
const month = (nowDate.getMonth() + 1).toString().padStart(2, '0');
const now = year + month;

const ExpirationDateFormSection = ({ ...props }) => {
const { changeExpiration } = props;
const initializeInputFieldState = (length: number) => {
const obj: InputStates = {};
for (let i = 0; i < length; i++) {
obj[i] = {
value: '',
hasError: false,
hasFocus: i === 0,
isFilled: false,
};
}
return obj;
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

코드에서는 현재 날짜(nowDate)와 입력된 유효기간을 각각 문자열로 처리하고, 이를 숫자로 변환하여 비교하고 있는데, 이렇게 관리하면

문자열을 숫자로 변환할 때 앞에 오는 0이 제거되어버리거나, 문자열의 형식이 예상치 못한 방식으로 변환될 수 있을거같아요

이방식보다 Date 객체를 사용하여 날짜를 비교하면 어떨까요?

예를들면

const inputExpirationDate = new Date(`20${inputYear}-${inputMonth}-01`);
const currentDate = new Date();

if (inputExpirationDate < currentDate) {

이런식도 괜찮을거 같구요

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

와우! new Date에 원하는 날짜를 넣으면 Fri Apr 01 2011 09:00:00 GMT+0900 (한국 표준시) 이런 형식으로 출력되는 것은 처음 알았네요!
그리고 Date 객체끼리 비교할 수도 있군요..! 훨씬 간결하고 좋은 것 같습니다. :)
코멘트 남겨주신 것처럼 수정하도록 하겠습니다!

@JeongBin0227
Copy link
Copy Markdown

소하 추가적으로, 소하가 만든 카드 UI랑
image
피그마랑 다른거 같아요

@soi-ha
Copy link
Copy Markdown
Author

soi-ha commented Apr 19, 2024

앗..! 말씀드린다는 것을.. 제가 잊어버렸네요..ㅠ 혼란스럽게 해드려서 죄송합니다..!
피그마에서 제시한 UI에 추가적으로 디자인을 더 입혔습니다.
아무래도 미션에는 디자이너가 따로 존재하지 않아서 디자인 수정에 대해 논의를 할 수 없으니, 페어와 상의하여 디자인을 조금 변경시켜서 진행했습니다.
우테코 미션에서 이런 디자인 수정은 지양하는 것이 좋을까요?

@soi-ha
Copy link
Copy Markdown
Author

soi-ha commented Apr 21, 2024

안녕하세요, 케빈!
리뷰 남겨주신 거 잘 봤습니다 :)
케빈이 친절하게 수정 코드까지 같이 코멘트를 해주셔서, 어떻게 수정해야 할지 바로 감을 잡을 수 있었습니다. 너무 좋았어요..ㅜ 감사합니다!

피드백 반영 사항

질문

step2 요구사항에 커스텀 훅을 만드는 것이 존재하고 이번에 긴 코드라인을 줄이기 위해서 useInput이라는 커스텀 훅을 만들어봤습니다.
커스텀 훅이라는 개념을 들어만 봤었고 사용해 보는 것은 이번이 처음입니다..! 그래서 재 리뷰 요청하는데 시간이 조금 걸렸습니다..

ExpirationDateFormSection, NameFormSection, CardNumbersFormSection이 3가지에 공통적으로 사용되고 있는 코드들을 모아 useInput이라는 커스텀 훅을 만들었습니다.
커스텀 훅을 이렇게 사용하는 것이 맞을까요?

그리고 useInput에 여러 setBlur나 resetErrors와 같은 여러 기능들이 존재합니다.
useInput 파일에 모두 작성해도 괜찮은지, 아니면 이것 또한 여러 파일로 분리해서 사용하는 것이 좋을지 궁금합니다!

@JeongBin0227
Copy link
Copy Markdown

앗..! 말씀드린다는 것을.. 제가 잊어버렸네요..ㅠ 혼란스럽게 해드려서 죄송합니다..!
피그마에서 제시한 UI에 추가적으로 디자인을 더 입혔습니다.
아무래도 미션에는 디자이너가 따로 존재하지 않아서 디자인 수정에 대해 논의를 할 수 없으니, 페어와 상의하여 디자인을 조금 변경시켜서 진행했습니다.
우테코 미션에서 이런 디자인 수정은 지양하는 것이 좋을까요?

크게 상관은 없다고 생각하는데, 이 부분은 수정하면 근거랑 같이 얘기해주는것도 좋을거 같아요~

@JeongBin0227
Copy link
Copy Markdown

ExpirationDateFormSection, NameFormSection, CardNumbersFormSection이 3가지에 공통적으로 사용되고 있는 코드들을 모아 useInput이라는 커스텀 훅을 만들었습니다.
커스텀 훅을 이렇게 사용하는 것이 맞을까요?

그리고 useInput에 여러 setBlur나 resetErrors와 같은 여러 기능들이 존재합니다.
useInput 파일에 모두 작성해도 괜찮은지, 아니면 이것 또한 여러 파일로 분리해서 사용하는 것이 좋을지 궁금합니다!

useInput 훅을 보면 입력 상태를 관리하고, 입력 값의 유효성 검사, 포커스 및 에러 상태의 처리를 포함하고 있는데, 다양한 기능이 잘 녹아져 있는거 같고, 잘 작성된 커스텀 훅 이라고 생각합니다.

useInput 파일에 모두 작성하였지만, 한 훅은 한 가지 기능만을 담당해서 큰 문제는 없어 보입니다.
왜냐하면 모든 기능이 입력 처리와 직접적으로 관련되어 있기 때문에, 같은 파일에 포함시키는 것이 의미가 있고, 하나의 훅 내에 필요한 모든 기능이 포함되어 있어, 사용하는 측에서는 추가적인 훅을 찾거나 import할 필요가 없기 때문입니다.

하지만 앞으로 계속적으로 추가가 되거나, 다른 기능(포커스 기능등)이 추가된다면 그때 분리해서 재사용성을 높여도 좋을거 같아요

Copy link
Copy Markdown

@JeongBin0227 JeongBin0227 left a comment

Choose a reason for hiding this comment

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

안녕하세요 소하!

수정해주신 부분 모두 확인하였습니다. 추가로 질문주신 부분도 답변 남겼으니 편하게 읽어보시면 좋을거같아요

이번 1단계는 여기서 머지하고 2단계에서 만나요~ 💯

@JeongBin0227 JeongBin0227 merged commit ffb2d2c into woowacourse:soi-ha Apr 22, 2024
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.

3 participants