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

Textfield, OutlinedTextField 구현 #29

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

kangyuri1114
Copy link
Member

@kangyuri1114 kangyuri1114 commented Jan 19, 2025

Handy Component로는 TextField가 사용되고 여러 TextField 구현을 위해 OutlinedTextField를 구현했습니다

  • YDS에서는 OutlinedTextField 를 material꺼를 사용 중이었어서 직접 구현했습니다.
  • TextArea는 아직 구현 중이라 Textfield, OutlinedTextField 만 봐주시면 감사하겠습니다.

제가 다음주 개발이 불가능해서 미리 올리고 코드리뷰 받아두는 게 시간상 좋을 것 같아 textArea 개발 전에 textField 먼저 올립니다.

_2025_01_19_19_35_56_498.mp4

@kangyuri1114 kangyuri1114 requested a review from a team January 19, 2025 10:39
@kangyuri1114 kangyuri1114 self-assigned this Jan 19, 2025
.padding(20.dp),
verticalArrangement = Arrangement.spacedBy(20.dp)
) {
val (text, onValueChange) = remember { androidx.compose.runtime.mutableStateOf("") }
Copy link
Contributor

Choose a reason for hiding this comment

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

요거만 임포트해서 줄여주시면 좋을거 같습니다

modifier = Modifier.wrapContentSize().padding(20.dp),
verticalArrangement = Arrangement.spacedBy(20.dp)
) {
val (text, onValueChange) = remember { androidx.compose.runtime.mutableStateOf("") }
Copy link
Contributor

Choose a reason for hiding this comment

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

이거도요

Copy link
Contributor

@Gael-Android Gael-Android left a comment

Choose a reason for hiding this comment

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

👍

@kangyuri1114 kangyuri1114 requested a review from a team January 19, 2025 11:07
Comment on lines +48 to +56

Row(
modifier = modifier
.fillMaxWidth()
.clip(RoundedCornerShape(12.dp))
.background(HandyTheme.colors.bgBasicLight)
.border(1.dp, borderColor, RoundedCornerShape(12.dp))
.padding(start = 16.dp, end = 12.dp, top = 12.dp, bottom = 12.dp)
) {
Copy link
Member

Choose a reason for hiding this comment

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

Surface를 사용하면 간단하게 될 것 같아요!

Comment on lines +105 to +110
@Composable
fun getTextFieldStyle(
enabled: Boolean,
isError: Boolean,
isFocused: Boolean
): TextFieldStyle {
Copy link
Member

Choose a reason for hiding this comment

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

TextFieldStyle의 접근 지정자를 public으로 두신 이유가 있을까요? internal이나 private이어도 될 것 같아서요.

이 함수는 외부에서 사용될 함수가 아니라면 internal이나 private로 해주시면 좋을 것 같습니다.

Comment on lines +81 to +83
trailingIcon?.let {
Icon(
imageVector = trailingIcon,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
trailingIcon?.let {
Icon(
imageVector = trailingIcon,
trailingIcon?.let {
Icon(
imageVector = it,

it으로 하는 게 통일성 있을 것 같아요

interactionSource = interactionSource,
onClick = onClickTrailingIcon
)
.padding(start = 12.dp)
Copy link
Member

Choose a reason for hiding this comment

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

padding보다는 Spacer가 명시적이어서 나을 것 같긴 한데 어떻게 생각하시나요?

Copy link
Member

Choose a reason for hiding this comment

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

그러면 TextArea는 별도의 PR로 올려주시고 여기서는 제외하는 게 좋을 것 같습니다!

Comment on lines +12 to +13
import com.yourssu.handy.compose.foundation.HandyTypography
@Composable
Copy link
Member

Choose a reason for hiding this comment

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

한 줄 띄워주세요~!

Comment on lines +49 to +54
Row(
modifier = modifier
.fillMaxWidth()
.clip(RoundedCornerShape(12.dp))
.background(HandyTheme.colors.bgBasicLight)
.border(1.dp, borderColor, RoundedCornerShape(12.dp))
Copy link
Member

Choose a reason for hiding this comment

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

여기두 Radius.M.dp 사용할 수 있지 않을까요..?!

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.

4 participants