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

[FE] Input 컴포넌트 리팩터링 #946

Open
wants to merge 11 commits into
base: FE/dev
Choose a base branch
from
Open

[FE] Input 컴포넌트 리팩터링 #946

wants to merge 11 commits into from

Conversation

dle234
Copy link
Contributor

@dle234 dle234 commented Nov 3, 2024

연관된 이슈

구현한 기능

#932

상세 설명

색깔이 생각보다 많이 변해서 일단
image
이렇게 했는데 나중에 맞춰봐요!

Input�이 내부에 속해있는 느낌이라 레이아웃을 Input 으로 네이밍 하는게 약간 어색하다고 생각해서 일단 InputGroup 으로 했습니다. 혹시 더 좋은 네이밍 있으면 말해주세요!!

export const InputGroup = Object.assign(Layout, {
  Label,
  Input,
  Message,
  Content,
});
image 대충 그렸는데 Content 는 display flex 하는 레이아웃 이고, Input 에 reset button 있으면 좋을 것 같아서 추가했어요(useInput 에도 resetValue 함수추가). resetValue props 에 함수 넣어주면 버튼 생겨요. 값 입력 시 버튼 생기고 나중에 IconButton 생기면 수정할예정입니다!

나머지 내용은 화요일에 가서 자세히 설명해드릴게요

@dle234 dle234 added the ⚙️ refactor 리팩터링 작업 label Nov 3, 2024
@dle234 dle234 self-assigned this Nov 3, 2024
Copy link
Contributor

@greetings1012 greetings1012 left a comment

Choose a reason for hiding this comment

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

수고하셨습니다!
수정 사항 남겨놓았으니, 시간날 때 확인해주세요~

@@ -49,8 +52,9 @@ export const Item = styled.li<{ $isChecked: boolean }>`
}
`;

export const CustomInputMessage = css`
top: 4rem;
export const Form = styled.form`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export const Form = styled.form`
export const Layout = styled.form`

width?: string;
height?: string;
borderRadius?: string;
focusColor?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

여기서 hex값보다는 theme.color를 받는 게 더 좋을 것 같아요!

Comment on lines +16 to +19
status = 'DEFAULT',
fontSize = theme.fontSize.sm,
$css,
children,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
status = 'DEFAULT',
fontSize = theme.fontSize.sm,
$css,
children,
$css,
status = 'DEFAULT',
fontSize = theme.fontSize.sm,
children,

props 와 정렬 순서가 같았으면 좋겠어요!

Comment on lines +12 to +18
status?: InputStatus;
$css?: ReturnType<typeof css>;
width?: string;
height?: string;
borderRadius?: string;
focusColor?: string;
onReset?: () => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
status?: InputStatus;
$css?: ReturnType<typeof css>;
width?: string;
height?: string;
borderRadius?: string;
focusColor?: string;
onReset?: () => void;
$css?: ReturnType<typeof css>;
status?: InputStatus;
width?: string;
height?: string;
borderRadius?: string;
focusColor?: string;
onReset?: () => void;

$css 가 가장 위로 올라갔으면 좋겠어요! (가독성 문제)

onReset?: () => void;
}

const Input = forwardRef<HTMLInputElement, InputProps>(
Copy link
Contributor

Choose a reason for hiding this comment

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

저희 forwardRef 제거하기로 했으니 제거합시다!

$width={width}
$height={height}
$borderRadius={borderRadius}
$focusColor={focusColor}
Copy link
Contributor

Choose a reason for hiding this comment

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

그냥 color 로 받아도 되지 않을까요?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚙️ refactor 리팩터링 작업
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants