[YDS-#218] Atom - SuffixTextField 구현 #268
Conversation
| isError: Boolean = false, | ||
| isEnabled: Boolean = true, | ||
| placeHolder: String = "", | ||
| suffixLabel: String = "", |
There was a problem hiding this comment.
SuffixTextField니까 suffixLabel 인자의 기본값을 주지 않는 것이 어떨까요? 그리고 text도 마찬가지로 기본값이 없는 것 제안해봅니다!
version.properties
Outdated
| @@ -1,2 +1,2 @@ | |||
| versionName=2.5.10 | |||
| versionName=2.5.11 | |||
There was a problem hiding this comment.
컴포넌트 하나가 추가된 PR이니까 중간의 번호를 올리는 게 좋을 것 같아요! 2.6.0
| text: String = "", | ||
| modifier: Modifier = Modifier, | ||
| isError: Boolean = false, | ||
| isEnabled: Boolean = true, |
There was a problem hiding this comment.
isDisabled로 통일되면 좋을 것 같아요
https://www.notion.so/yourssu/SimpleTextField-0ef66176557a487fa5a449d9e9f8ee6a?pvs=4
|
@cometj03 말씀해주신 거 반영해서 수정했습니다! 어프루브 주시면 머지할게요! |
| isError: Boolean = false, | ||
| isDisabled: Boolean = true, | ||
| placeHolder: String = "", | ||
| suffixLabel: String, |
There was a problem hiding this comment.
기본값 없는 파라미터는 맨 위로 올리는게 통일성있어 보입니다!
코틀린 문서에서 찾아봤는데 그런 얘기는 못 찾았지만, 대부분의 컴포저블에서 기본값 없는 파라미터들이 먼저 나오는 모습이 보이네요. 아마 Named Arguments를 사용하지 않더라도 자연스럽게 느껴지도록 기본값 없는 파라미터를 먼저 쓰도록 하는 것 같아요 !
| textColor = YdsTheme.colors.textSecondary, | ||
| ), | ||
| isError = isError, | ||
| enabled = isDisabled, |
There was a problem hiding this comment.
기존 사용되는 enabled로 사용하려면, !isDisabled로 사용하는 것이 맞지 않을까요!?
There was a problem hiding this comment.
앗 맞네요! 제가 놓쳤습니다😅 피드백 반영했습니다!
| Row(modifier = Modifier.padding(top = 8.dp)) { | ||
| Spacer( | ||
| modifier = Modifier | ||
| .width(16.dp), |
There was a problem hiding this comment.
엇 제가 피그마 확인해봤을 땐 8dp던데 16dp였을까요..?
There was a problem hiding this comment.
확인해보니 8dp가 맞네요!! 다음엔 더 꼼꼼히 보겠습니닷
There was a problem hiding this comment.
현재 다른 TextField들도 hintText는 8dp인데 16dp로 되어있는 것 같습니다..! 같이 수정해놓을까요??
Summary
Describe your changes
Issue
To reviewers
PR 올리기 전 체크 리스트