-
Notifications
You must be signed in to change notification settings - Fork 208
[1단계 - 페이먼츠] 써밋(이우혁) 미션 제출합니다. #438
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
Conversation
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
-> 여러 input이 있다면 input마다 개별 label을 매핑하는 게 맞다고 생각해요. 즉, input 개수만큼 label을 만든다가 웹 접근성(a11y) 표준에 부합하는 방식입니다. 각각의 input은 고유한 label이 있어야 스크린 리더 사용자도 정확하게 각 input의 의미를 이해할 수 있습니다. 그래서 label의 htmlFor 속성과 input의 id를 1:1로 연결하는 게 원칙입니다. 결론은, 지금 선택하신 input마다 label을 하나씩 매핑 방식은 올바른 접근이라고 생각해요 |
JeongBin0227
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.
안녕하세요! 써밋 다시 만나서 반가워요
전체적으로 코드를 잘 짜신거 같아요 👍 학습목표에 맞춘 내용과 코드 읽어가면서 수정하면 좋을 것들을 남겨두었으니 읽어보시면 될거같아요! 근데 pr 코멘트에도 남겼는데, 파일 구조가 꺠져서 보입니다. src 안에 또 src가 있고 components 가 중복된거 같은데 이부분 먼저 해결보시면 좋을거 같아요!
페이먼츠 미션하느라 고생많으셨어요~ 또다시 열심히 해봐요~ 💯
| addCardState: { | ||
| cardNumberState, | ||
| handleCardNumberChange, | ||
| expireDate, | ||
| handleExpireMonthChange, | ||
| handleExpireYearChange, | ||
| handleExpireMonthBlur, | ||
| CVCState, | ||
| handleCVCChange, | ||
| }, | ||
| }: AddCardFormProps) { |
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.
이 패턴이 저는 좀 어색하게 느껴져요.
현재 addCardState를 props에서 받아서 다시 내부에서 한번 더 구조분해 하고 이쓴ㄴ데, 차라리 props에서 바로 꺼내는 게 아니라, 컴포넌트 바디 안에서 꺼내는 패턴으로 바꿔보면 어떨까요?
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.
왜냐하면 props는 이름만 맞으면 되고, 실제 사용하는 변수는 내부에서 관리하는 게 가독성이 더 좋다고 생각해요
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.
말씀해주신 방식이 더 가독성이 좋은 것 같습니다🫡
| <CardInputBox | ||
| title="결제할 카드 번호를 입력해 주세요" | ||
| guideText="본인 명의의 카드만 결제 가능합니다." | ||
| InputComponents={ |
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.
InputComponents 보다 하나니까 InputComponent 는 어떨까요?
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.
인풋들이 들어있는 컴포넌트여서 복수를 생각했던 것 같은데 인풋들을 모은 컴포넌트 자체는 하나이기 때문에 말씀해주신 prop명이 더 맞는 것 같습니다👍👍👍
|
|
||
| export const WithValidationTest: Story = { | ||
| render: () => { | ||
| const { CVCState, handleCVCChange } = useControlledCVC(); |
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.
저는 항상 스토리북 사용할때 이런게 좀 고민이더라구요.
보면 훅을 직접 호출해서 컴포넌트를 생성하고 있는데, 이러면 스토리북 args 기반 설계원칙이랑 멀어진다고 생각해요
지금 개선할수있는 방법은 Story의 args만 사용해서 모든 상태를 만들고 필요한 경우 play에서 조작하는건데, 즉 Story는 순수하게 props → 컴포넌트 생성만 담당하고, 로직은 가능한 한 별도로 빼는 걸 지향하는걸 추천하는데 써밋 생각은 궁금하네요
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.
사실 이번에 스토리북을 처음 써봐서 페어랑 고민했던 부분이긴 한데 Default, Error 스토리는 args를 통해 정적인 props 상태를 정적으로 테스트하였고 WithValidationTest에서는 훅을 사용해서 동적인 부분을 테스트하기 위한 목적으로 작성했던 것 같습니다..!
React Testing Library나 Playwright 같이 따로 동적인 테스트를 할 수 있는 라이브러리를 사용한다고 하면 스트리북에서는 정적인 테스트만 진행했을 것 같은데 그렇지 않아서 스토리북에서 동적 테스트를 진행했던 것 같습니다!
| <p className={styles.cvcInputs}> | ||
| <Input | ||
| id="cvc-input" | ||
| type="text" |
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.
여기 text 가 아니라 number 아닐까요?
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.
저도 처음에는 number 타입이 더 목적에 맞고 시맨틱하다고 생각하여서 number 타입으로 지정해서 미션을 진행하였는데, number 타입은 한글이 한 글자 입력이 가능한데 그 한 글자를 입력했을 때 onChnage 이벤트가 동작하지 않아 따로 유효성 검사를 하지 못하는 문제가 있어 text 타입으로 변경하고 숫자 형식이 맞는지에 대한 유효성 검사를 하는 것으로 대체하였던 것 같습니다!
- input type number로 변경, onChange 이벤트 발생 시 콘솔 출력하는 로직 추가
const handleCardNumberChange = useCallback((key: CardNumberInputKey, value: string) => {
console.log("onChang 이벤트 발생!");
// another code..
}, []);
<Input
type="number"
isError={cardNumberState[inputKey].isError}
value={cardNumberState[inputKey].value}
onChange={(e) => handleCardNumberChange(inputKey, e.target.value)}
/>- 한글을 입력했을 때
onChange이벤트가 발생하지 않는 영상(숫자 입력 시 이벤트 발생)
2025-04-18.11.47.09.mov
| import { useCallback, useState } from "react"; | ||
|
|
||
| const useControlledCVC = () => { | ||
| const [CVCState, setCVCState] = useState({ |
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.
CVCState 타입도 추가해보면 어떨까요?
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.
앗 그러네요 다른 훅들은 타입을 지정해 줬는데 CVC는 빠뜨렸네요.. 타입 추가하는 것으로 수정하였습니다🫡
| ); | ||
| }, | ||
| play: async ({ canvasElement }) => { | ||
| const inputElement = canvasElement.querySelector( |
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.
보통 #card-number-first-input 처럼 id 를 통해 가져오기도 하지만, 유지보수하면서 변경될수도 있어서 role이나 label 통해가져오는게 좋아요~
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 속성이나 data-id 같은 속성으로 DOM을 셀렉팅하는 것에 대해 고민이 있어서 이것도 여쭤보려고 했는데 깜빡하고 PR 본문에 작성 못했던 것 같네요..😭
꼼꼼한 리뷰 감사합니다🙇♂️
| </Label> | ||
| <Input | ||
| id={`card-number-${inputKey}-input`} | ||
| type="text" |
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.
여기서 추가로 숫자만 입력하고 싶으면
inputMode="numeric"
를 활용할수도 있을거 같아요~
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.
모바일 환경까지 고려하는게 좋은 것 같습니다👍👍👍
| <span | ||
| id={`${expireKey}-error-message`} | ||
| className={styles.errorMessage} | ||
| > | ||
| {expireDate[expireKey].errorMessage} | ||
| </span> |
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.
expireKey 가 없을수도 있으니,
{expireDate[expireKey].errorMessage && (
<span
id={`${expireKey}-error-message`}
className={styles.errorMessage}
>
{expireDate[expireKey].errorMessage}
</span>
)}
이렇게 변경해보면 어떨까요?
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.
헉 맞네요! 이 부분은 놓친 것 같습니다.. 감사합니다👍👍👍
| <Label htmlFor={`expire-${expireKey}-input`} isHidden={idx !== 0}> | ||
| 유효 기간 | ||
| </Label> | ||
| <Input |
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.
input 중의 pattern 도 활용할수 있는데 공부해보고 여기서도 활용해볼수있을지 확인해보면 좋을거 같아요
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.
input 태그의 pattern 속성으로 유효성 검사를 해도 유저가 개발자 도구를 수정했을 때를 대비해서 js로 유효성 검사를 해야하는게 안전하다는 생각이 들었던 것 같은데 그래도 추가적으로 pattern 속성을 통해 유효성 검사를 해주는게 좋을까요??
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.
<Input
type="text"
pattern="[0-9]*"
...
저는 이런식으로 태그의 도움을 받을 수 있다면 추가하는게 좋다고 생각해요
그렇게 하면 UX 적으로도 입력 제한을 더 자연스럽게 유도할 수 있다고 생각합니다.
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.
브라우저에서도 유효성 검사하고, JS에서도 유효성 검사하는게 더 안전하고 좋은 UX일 수 있겠네요!
좋은 의견 감사합니다🙇♂️
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
Co-authored-by: spoyodevelop <[email protected]>
-> 저는 보통 hidden 을 사용합니다. 예전에 form 요청할때 사용한 방법과 비슷한데, hidden 을 쓰면 시각적으로는 완전히 숨기되, 스크린리더는 정상적으로 읽을 수 있도록 해줍니다! |
-> 결국 정리하면 flat 하냐, depth를 유지하나 일거 같아요. 사실 요런 문제는 정답이 있기보단 기준을 잡는게 중요할거 같아요 사용 범위와 의존관계를 중점으로 두고 싶다 -> depth 가 좋음 이런식으로 기준이 중요할거 같아요 그래서 정리하면 이 경우에는 DOM 트리 구조에 맞게 depth를 두는 방향이 더 좋아보여요. 왜냐하면 해당 미션은 다른곳에서 재사용 되서 사용되는 파일보다는 명확한 의존 관계도 많고,앞으로 개발을 더 많이 해야하기 때문에 depth가 좋아보여요 |
-> 네! 맞아요 요런식의 방식도 많이 사용하기에 하나의 컴포넌트는 위처럼 바꿔서 공부해봐도 좋을거같아요! |
-> 네 저는 여전히 동일하게 보이네요.. 이런적이 첨이라 신기하네요.. |
좋은 것 같습니다~! const meta = {
title: "Component/Input",
component: Input,
args: {
type: "text",
placeholder: "카드 번호를 입력하세요",
disabled: false,
readOnly: false,
minLength: 0,
maxLength: 30,
isError: false,
},
} satisfies Meta<typeof Input>;
|
JeongBin0227
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.
안녕하세요 써밋~
반영도 빠르게 해주셨는데, 리뷰를 달자마자 바로 빠르게 또 코멘트를 달아주셨네요 👍
덕분에 빠르게 1단계 미션은 여기서 머지하고 2단계를 같이 보면 될거같아요
경로 문제는.. 2단계에서 다시 확인해봐야할거 같아요!
1단계 미션 고생하셨어요~ 2단계에서 만나요!









안녕하세요 케빈!
레벨 1 점심 뭐 먹지 미션에서도 많이 배웠는데, 이번 페이먼츠 미션 리뷰도 받을 수 있어서 너무 좋은 것 같네요🙃
PR 본문에 케빈의 의견이 궁금한 부분들은 작성하였습니다🫡
이번 미션도 잘 부탁드립니다~🙇♂️
🎯 미션 소개
이번 미션을 통해 다음과 같은 학습 경험들을 쌓는 것을 목표로 합니다.
🕵️ 셀프 리뷰(Self-Review)
제출 전 체크 리스트
리뷰 요청 & 논의하고 싶은 내용
1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
스타일 라이브러리 선정(Module CSS vs Styled-Component vs Emotion)
위와 같은 이유로 러닝 커브가 없이 기본 css 문법으로 동작하는
Module CSS스타일링을 선정하였습니다!상태 관리(커스텀 훅)
사실 지금 사용하는 커스텀 훅이 재사용성은 떨어지지만 단순히 컴포넌트 레벨에서
useState로 관리했을 때 가독성이 떨어지는 이슈가 있어서카드 번호,유효 기간,CVC인풋 상태 관리에 대한 커스텀 훅을 만들어서 사용하였습니다!2) 이번 리뷰를 통해 논의하고 싶은 부분
폴더 구조
위 폴더 구조와 같이 특정 컴포넌트에서만 사용되는 컴포넌트는 DOM 트리 구조에 맞게 해당 컴포넌트의
components폴더에 위치하였는데 depth가 깊어지는 것 같은 느낌을 받았던 것 같습니다..!😮AddCard폴더 안에 flat 하게 가는게 좋을까요? 아니면 지금처럼 DOM 트리 구조에 맞게 depth를 두는게 좋을까요??정답은 없겠지만 케빈의 의견이 궁금합니다!!💭
Label 태그 ↔ Input 태그 매핑(웹 접근성)
기존에는
input태그 4개에label태그 1개를 사용하고 있었는데 이렇게 사용하면label태그의htmlFor속성을 하나의 인풋만 매핑할 수 있는 문제가 있었습니다.페어와 논의하였을 때 이 방식은 시맨틱하지 않다는 생각이 들었고
input태그 개수만큼label태그를 매핑하는게 맞다고 판단하여 여러label태그 중 첫 번째label태그만 화면에 보여주고 나머지label태그는 렌더링하지만 화면에 보이지 않고 스크린 리더에만 읽힐 수 있게 설정 하였습니다.이런 경우에는 어떤 방식을 사용하는게 일반적인지 궁금하고🤔 케빈은 화면에 보이지 않고 스크린 리더에만 읽힐 수 있게 스타일링해야 할 때 어떤 방식을 사용하는지도 궁금합니다~!🙃
✅ 리뷰어 체크 포인트
1. 컴포넌트 설계
2. 상태 관리
3. Storybook 활용
4. UI/UX