Skip to content
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

feat: TextField 컴포넌트 구현 #151

Merged
merged 12 commits into from
Oct 9, 2024
Merged

feat: TextField 컴포넌트 구현 #151

merged 12 commits into from
Oct 9, 2024

Conversation

fecapark
Copy link
Collaborator

@fecapark fecapark commented Aug 25, 2024

1️⃣ 어떤 작업을 했나요? (Summary)

TextField 컴포넌트를 구현했어요.

2024-08-25.16.21.46.mov

2️⃣ 추후 작업

아무래도 제공하는 props나 기능적으로 @seocylucky 가 구현한 Textarea 컴포넌트와 기능적으로 일치할 거 같은데요,

Helper Text 부분을 저는 compound component 로 구현한 것과는 달리,
@seocylucky 는 props 방식으로 구현했어요.

// 저의 방식
<TextField>
  <TextField.HelperText>도움말</TextField>
</TextField>

// 체리 방식
<TextField helperText="도움말" />

이 부분에 일관성을 가지기 위해 얘기할 시간이 필요해보여요.


++ 그리고 머지할 때 index.ts 컨필릭 해결할게요

4️⃣ 체크리스트 (Checklist)

  • main 브랜치의 최신 코드를 pull 받았나요?
  • 빌드 및 다른 프로젝트에서의 구동 테스트 완료

Sorry, something went wrong.

Copy link
Collaborator

@nijuy nijuy left a comment

Choose a reason for hiding this comment

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

리뷰가 쪼끔 늦었습니다 ts-pattern 관련 메시지도 이거 내고 확인할게요!
고생하셨습니다~~~~

ThumbsUpDoubleThumbsUpGIF

그리고 머지할 때 index.ts 컨필릭 해결할게요

안그래도 거기가 제일 먼저 보였는데 알잘딱따구리네요

<StyledTextFieldContainer>
<StyledTextFieldInputContainer>
<StyledTextFieldInput
{...props}
Copy link
Collaborator

Choose a reason for hiding this comment

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

defaultValue...props 안에 있어서 value랑 defaultValue 중에 하나만 쓰라고 콘솔 경고가 뜨고 있던데!
children~ onClearButtonClick 처럼 defaultValue도 한번 별도로 빼줘야 (?) 할 거 같아요

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

옹 이런 문제가 ㄷㄷ
확인했습니다

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

inputRef.current.dispatchEvent(new Event('input', { bubbles: true }));
};

const onClickHadler = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

함수명에 n도 끼워주세요 (●ˇ∀ˇ●)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

혀가 짧아서...
장난이고 수정할게요 ㅋㅋㅋ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

$isError ? theme.semantic.color.lineStatusNegative : theme.semantic.color.textBasicTertiary};
`;

export const StyledClearButton = styled.button`
Copy link
Collaborator

Choose a reason for hiding this comment

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

클리어 버튼 색상 적용도 부탁함니다

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

width: 100%;
`;

export const StyledTextFieldInput = styled.input<{ $isError: boolean }>`
Copy link
Collaborator

Choose a reason for hiding this comment

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

음............. textarea는 caret-color가 따로 지정되어 있었는데
textfield는 적용된 곳도 아닌 곳도 있어서 뭐가 맞는지 모르겠네요 😐
(아마 해야할 거 같긴 한데 확인차 피그마 코멘트 남겨뒀습니다!!)

답변에 따라 caret-color 적용이 필요해질 수도 있을 거 같아요?!

image
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

와 이건 몰랐는데

Copy link
Collaborator

Choose a reason for hiding this comment

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

이거 피그마 코멘트 확인하셨을 거 같은데! 캐럿 컬러 적용하는 게 맞대요 ◠‿◠ 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

import { styled } from 'styled-components';

export const StyledTextFieldContainer = styled.div`
width: 100%;
Copy link
Collaborator

Choose a reason for hiding this comment

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

요구사항 중 하나가 TextField의 가로 길이는 변형이 가능하다 이고,
지금은 가로 길이를 변형하려면 원하는 너비의 부모 컨테이너로 TextField를 감싸줘야 하는데
이 부분을 문서에 한번 언급해도 좋을듯 합니다!!!!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

그게 좋을듯하네요

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Member

@seocylucky seocylucky left a comment

Choose a reason for hiding this comment

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

수고하셨습니다!!!
뭔가 구조적으로 봤을 때는 Compound Component가 더 좋은 것 같고...
직관적으로 봤을 때는 props 방식이 더 직관적인 것 같은데
나중에 유지보수 측면에서 따져봤을 때는 Compound Component 사용이 더 좋을 것 같긴해요!

(Compound Component로 정해진다면) 좀 큰 수정 작업이라 추후 제가 Compound Component 방식으로 따로 이슈 파서 리팩토링을 해야겠네여

<TextField placeholder="이름을 입력해주세요...">
<TextField.Label>이름</TextField.Label>
<TextField.HelperText>신분증상 이름을 입력해주세요.</TextField.HelperText>
</TextField.HelperText>
Copy link
Member

@seocylucky seocylucky Aug 28, 2024

Choose a reason for hiding this comment

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

Suggested change
</TextField.HelperText>
</TextField>

수정 필요해보여요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

<TextField placeholder="이름을 입력해주세요...">
<TextField.HelperText>신분증상 이름을 입력해주세요.</TextField.HelperText>
<TextField.Label>이름</TextField.Label>
</TextField.HelperText>
Copy link
Member

Choose a reason for hiding this comment

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

여기도!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@fecapark
Copy link
Collaborator Author

수고하셨습니다!!!
뭔가 구조적으로 봤을 때는 Compound Component가 더 좋은 것 같고...
직관적으로 봤을 때는 props 방식이 더 직관적인 것 같은데
나중에 유지보수 측면에서 따져봤을 때는 Compound Component 사용이 더 좋을 것 같긴해요!
(Compound Component로 정해진다면) 좀 큰 수정 작업이라 추후 제가 Compound Component 방식으로 따로 이슈 파서 리팩토링을 해야겠네여

굿굿 좋아요!!
이와 관련해서 제가 입장을 말해본적은 크게 없는거 같아서

https://yourssu.slack.com/archives/C079ZFY0QKX/p1726927575742079

관련 맥락을 위 슬렉 스레드에 남겨봤어요.

@@ -30,6 +30,8 @@ export const StyledTextFieldInput = styled.input<{ $isError: boolean }>`
$isError ? theme.semantic.color.lineStatusNegative : theme.semantic.color.lineStatusPositive};
border-radius: ${({ theme }) => theme.semantic.radius.m}px;

caret-color: ${({ theme }) => theme.semantic.color.lineStatusPositive};
Copy link
Collaborator

Choose a reason for hiding this comment

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

지금 error에서도 캐럿 컬러가 positive로 보이는 게 좀 어색하게 느껴져서요.
피그마에 error & typing 일 때 caret color가 명시된 건 아닌데 border color를 생각하면
error일 때는 lineStatusNegative가 적용되는 게 자연스러운 거 같아요

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

@nijuy nijuy Sep 22, 2024

Choose a reason for hiding this comment

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

어.. 제가 말씀 드렸던 부분은 StyledTextFieldInput의 caret-color인데요.
c6d5053 에서는StyledClearButton의 컬러가 바뀌면서 디자인 요구사항이랑 달라진 거 같아요
작업하신 부분의 의도를 잘 모르겠습니다 ㅜㅜ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

어 뭐야 저거 stash 했던건데 그대로 들어가있네요 죄송함다

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@fecapark fecapark requested a review from nijuy September 22, 2024 05:51
Copy link
Member

@seocylucky seocylucky left a comment

Choose a reason for hiding this comment

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

머지 레쯔고

@nijuy nijuy merged commit 6338dac into develop Oct 9, 2024
@nijuy nijuy deleted the feat/#149-textfield branch October 9, 2024 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: TextField 컴포넌트 구현
3 participants