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

Feature/mz 159 Textfield Component #30

Merged
merged 14 commits into from
Jan 4, 2024
Merged

Conversation

yangsooplus
Copy link
Member

💡 Issue

🌱 Key changes

  • SusuBasicTextField: 기본이 되는 TextField
  • SusuPriceTextField: SusuBasicTextFieldVisualTransformation을 적용
  • SusuUnderlineTextField: SusuBasicTextField을 활용하여 구현

✅ To Reviewers

스크린샷 2024-01-04 오전 2 13 52
  • TextFieldButton과 공유하는 clearIconButton을 분리해서 재사용했습니다.
  • SusuPriceTextField에 매우매우 큰 숫자를 입력하면 터집니다만...
    • PriceVisualTransformation에서 방지하기 위해 Long.MAX_VALUE를 기준으로 제한해봤는데, 간결히 말하면 Format된 문자열 계산하는 과정에서 오류가 발생해서 롤백했습니다
    • 데이터 정의 상 금액 단위가 int이고, int 최대 범위보다 넉넉하게 입력 가능합니다.
      • 21억이 넘는 돈을 경조사비로 주고 받는건 이재용 회장님밖에 없지 않을까요?

📸 스크린샷

SusuTextField SusuUnderlineTextField
textfield underlineTextfield

@yangsooplus yangsooplus self-assigned this Jan 3, 2024
@yangsooplus yangsooplus marked this pull request as ready for review January 3, 2024 17:23
@jinukeu
Copy link
Member

jinukeu commented Jan 4, 2024

default.mp4

"원" 누르면 터져용

Copy link
Member

@jinukeu jinukeu left a comment

Choose a reason for hiding this comment

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

중복된 코멘트는 제외했어요~!

class PriceVisualTransformation : VisualTransformation {
override fun filter(text: AnnotatedString): TransformedText {
val amount = text.text
val formatAmount = if (amount.isEmpty()) "" else DecimalFormat("#,###").format(amount.toLong())
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
val formatAmount = if (amount.isEmpty()) "" else DecimalFormat("#,###").format(amount.toLong())
val formatAmount = if (amount.isEmpty()) "" else DecimalFormat("#,###").format(amount.�to BigDecimal())

이렇게하면 Long보다 큰 숫자 입력받을 수 있어용

Copy link
Member Author

Choose a reason for hiding this comment

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

적용했습니다~ 이제 우다다 큰 수를 입력하면 터지지는 않게 되었습니다

val formatAmount = if (amount.isEmpty()) "" else DecimalFormat("#,###").format(amount.toLong())

return TransformedText(
text = AnnotatedString("${formatAmount}원"),
Copy link
Member

Choose a reason for hiding this comment

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

R.string.xxx로 못빼나용?

Copy link
Member Author

Choose a reason for hiding this comment

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

Composable 함수가 아니어서 stringResource()는 사용 못 하고, context.resources.getString()을 쓰려니 context를 끌고와야 합니다 PriceVisualTransformation 생성자에 넣어서 context를 끌고는 올 수 있겠네요

Copy link
Member

Choose a reason for hiding this comment

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

음 ... 저는 무조건 String Resource로 빼는 편인데 ... 어떡할까요?

https://ddioniii.tistory.com/24

Copy link
Member Author

Choose a reason for hiding this comment

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

시안 쭉 둘러보니 전체적으로 X원 형식을 많이 사용해서 리소스로 빼는 이점이 있겠네요
갑니다

Copy link
Member

Choose a reason for hiding this comment

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

어느건 리소스로 안빼고 어느건 리소스로 빼면 아무래도 일관성이 부족하니 한글이 들어간 경우라면 전부 리소스로 빼는 방향으로 갈게요잉! @syb8200 @yangsooplus

Copy link
Member Author

Choose a reason for hiding this comment

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

refactor: '원'을 string resource로 분리

갑자기 능지상승해서 이렇게 고쳤습니다 굳이 context 안 끌어와도 되네요

Copy link
Member

Choose a reason for hiding this comment

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

오 저도 context 쓰는거 찝찝했는데 이 방법 너무 좋네요!!

Copy link
Member Author

Choose a reason for hiding this comment

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

refactor: context.getString -> stringResource

생각해보니 아까 능지가 반단계만 상승했었네요

Comment on lines 58 to 65
if (text.isEmpty()) {
Text(
text = placeholder,
style = textStyle.copy(color = placeholderColor),
)
} else {
innerTextField()
}
Copy link
Member

Choose a reason for hiding this comment

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

text.isEmpty()일때 innerTextField가 없으니 커서가 안보이는 문제가 있어요!
innerTextField는 항상 보이게끔 수정 부탁드릴게요~

val limitationColor: Color,
val descriptionColor: Color,
) {
Unactive(
Copy link
Member

Choose a reason for hiding this comment

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

피그마에는 Unactive라고 나와있는데 문법상 Inactive가 맞는 표현이라서요~! Inactive로 수정 부탁드릴게요

Copy link
Member Author

Choose a reason for hiding this comment

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

사실 저도 계속 rename 하이라이팅 신경쓰였는데 안드로이드만의 비밀로 Inactive로 사용하죠 😆

Comment on lines +115 to +119
drawLine(
color = textFieldColor.underlineColor,
start = Offset(0f, size.height),
end = Offset(size.width, size.height),
strokeWidth = 1f,
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 100 to 102
if (text.isEmpty()) {
SusuUnderlineTextFieldColor.Unactive
} else {
Copy link
Member

Choose a reason for hiding this comment

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

Unactive상태는 Focused가 아닐때 Unactive일거에요.

Copy link
Member Author

Choose a reason for hiding this comment

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

이게 애매해서 혜원님께 확인하고 고쳐야 할 것 같아요!
내용을 작성하고 포커스를 없애면 PlaceHolder 급으로 글씨가 연해지는게 어색할 것 같아서..
Unactive = placeholder라고 받아들였거든요

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
Member Author

Choose a reason for hiding this comment

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

여쭤보았더니 아무것도 입력되지 않은 상태가 Unactive 맞네요!

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
Member Author

Choose a reason for hiding this comment

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

괜찮습니다 애초에 이거 이름 쓸 때 말고 쓰는 곳 없어보이거든요... (약간슬픔)

Comment on lines 97 to 102
val textFieldColor = if (isError) {
SusuUnderlineTextFieldColor.Error
} else {
if (text.isEmpty()) {
SusuUnderlineTextFieldColor.Unactive
} else {
Copy link
Member

Choose a reason for hiding this comment

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

else안에 또 if else가 있으니 가독성이 떨어지는거같은데 when문 사용해보는건 어때요?

style = limitationTextStyle.copy(color = textFieldColor.limitationColor),
)
}
if (description.isNotEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

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

description은 nullable하게 받는게 맞을거같아요.
description이 ""일때는 빈 문자열을 가진 Text가 존재해야하고 null일때는 Text자체가 없는 동작이 더 좋을거같습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

듣고보니 있다/없다를 표시하니까 nullable한게 더 어울리네요~ 수정했습니다~~

@yangsooplus
Copy link
Member Author

'원'을 누르면 터지는 것 해결했습니다~
동시에 '원'을 누른다면 숫자의 제일 끝에 커서가 위치하게 됩니다 (= '원' 앞에 커서가 위치합니다 )

Copy link
Member

@jinukeu jinukeu left a comment

Choose a reason for hiding this comment

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

오 코드 너무 깔끔하고 좋아요!!!
고생많으셨습니다 👍 💯

Copy link
Contributor

@syb8200 syb8200 left a comment

Choose a reason for hiding this comment

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

string 빼는 부분 능지성장 잘 봤습니다...👍 merge 가시죱..!!

@yangsooplus yangsooplus merged commit b9f5737 into develop Jan 4, 2024
1 check passed
@yangsooplus yangsooplus deleted the feature/MZ-159-textfield branch January 4, 2024 15:00
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