-
Notifications
You must be signed in to change notification settings - Fork 0
[TNT-110] 트레이너 회원 정보 입력 화면 구현 #22
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
Conversation
- 간격 추가 - 배경색 변경
- RoleSelectionScreen에 추가한 strings 적용
hoyahozz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
우선 1차로 보이는 것만 리뷰 적어뒀습니당! 이따 한 번 더 볼게요 ~~
| modifier = modifier, | ||
| modifier = modifier | ||
| .fillMaxWidth() | ||
| .padding(start = 12.dp, top = 20.dp, end = 12.dp, bottom = 12.dp), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vertical, horizontal 파라미터를 사용해도 될 것 같아요~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
top이 20.dp이고 bottom이 12.dp라 분리해서 써놨습니다 ;ㅅ;
|
|
||
| Spacer(Modifier.padding(top = 60.dp)) | ||
|
|
||
| TnTLabeledTextField( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요거 근데 maxLine 확인해보는게 좋을 것 같아요
이름 입력칸에서 줄바꿈하는 경우가 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이름 입력칸에서 두 줄이 되어야 하는 이유를 잘 모르겠...어요
근데 이건 기획 디자인 영역이라 한 번 여쭤보는게 좋을 것 같아여
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 여쭤보고 가능하면 한 줄로 바꿔오도록 하겠습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
한 줄로 수정했습니다👍 6e3c8c8
| /** | ||
| * 입력 값을 검사해 한글/영어/공백만 허용하고 특수문자는 제거 | ||
| */ | ||
| fun validateInput(input: String): String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
끼악 수정하겠습니다
|
|
||
| Text( | ||
| text = "${value.length}/$maxLength", | ||
| text = "${value.length}/${maxLength}자", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요것도 스트링 리소스로 빼야할 것 같아요!!
- BringIntoViewRequester 삭제
| ) { | ||
| // TODO 버튼 클릭 시 트레이너/트레이니 화면으로 이동 | ||
| TnTTopBar(onBackClick = {}) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
혹시 각 컴포저블 사이에 개행을 넣으시는 이유가 있으실까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compose 코드가 익숙하지 않아서 각 컴포저블 구분이 잘 되도록 개행을 넣고있었습니다!
개행을 안 넣는게 맞을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
프로그래밍 관점에서 개행이 들어가면, 현재 단락이 종료되고 새로운 개념이 시작된다는 느낌이라서 안넣는게 더 좋을 것 같다는 생각이긴 합니다 '-'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그렇군요.. 그러면 앞으로 각 컴포저블 사이에 개행 없이 작성하겠습니다! 수정해서 올려둘게요
| var text by remember { mutableStateOf("") } | ||
| var warningState by remember { mutableStateOf(false) } | ||
|
|
||
| warningState = text.length > maxLength |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
음.. 이렇게 상태를 주입해주면, 다른 상태가 변경되었을 때도 이 코드가 동작하게 될 것 같아요
지금 상태에선 text 만 있어서 상관없을 것 같기는 한데,
만약 해당 화면에서 다른 상태가 또 있다고 가정하면, 그 상태가 변경될 때마다 이 코드도 다시 실행될 것 같다는 생각이 드네요!
onValueChange 에서 텍스트 길이에 따라 warningState 를 변경시켜주는 방식이 좋을 것 같습니당~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아니면 derivedStateOf 에 대해 공부하고 적용하면 좋을 것 같네요 :->
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onValueChange = { newValue ->
val filteredText = validateInput(newValue)
text = filteredText
warningState = text.length > maxLength
},움 그럼 51번줄 삭제하고 이렇게 수정하면 될까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제안해드린 두 가지 방법 모두 고민해보시고, 더 나은 방법으로 선택하시면 될 것 같습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아 새로고침이 안 돼서 derivedStateOf코멘트는 확인을 못했습니다
derivedStateOf가 유효성 검사에 더 맞는 것 같습니다! 새로 알아가네용
해당 방법 사용해서 수정해두겠습니다
| import co.kr.tnt.designsystem.theme.TnTTheme | ||
|
|
||
| @Composable | ||
| fun SetTrainerProfileScreen( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
흐음 뭔가 Set 이라는 표현은 동사에 가까워보이는데 명사로 이름을 지을순 없을까용 ㅎ.ㅎ
TrainerProfileFormScreen ?..
한 번 생각해봅시다!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TrainerProfileSetupScreen 으로 수정하겠습니다
다른 것들도 명사로 다 수정해둘게요!
| } | ||
|
|
||
| @Composable | ||
| private fun SetProfileImage( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기도 이름 바꿔봅시다!
컴포넌트명을 지을 땐, 특별한 경우가 아니라면 최대한 다른 화면에서도 '언제든지' 사용할 수 있다고 가정하고 이름을 짓는게 좋아요!
요 컴포넌트는 실제로 나중에 '내 프로필' 화면에서도 사용될 수 있는 컴포넌트겠네요!
범용적인 이름을 생각해봅시다!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
옹 내 프로필에서도 쓸 수 있겠네요..!
안그래도 트레이니 화면에서도 쓰이기 때문에 해당 부분 TNT-113 브랜치에서 ProfileImageSection으로 분리해서 구현하고 있었습니다
이번 PR에서는 일단 그대루 둬도 될까요?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵넵 다음 PR에서 적용해봅시다!
| modifier = modifier | ||
| .fillMaxSize() | ||
| .imePadding() | ||
| .verticalScroll(rememberScrollState()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
엇 이거 스크롤은 왜 들어간건가요? 이러면 탑바도 같이 스크롤될 것 같은데여 !?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
기기로 테스트 하다보니 imePadding()만 넣으면 TextField가 키보드에 가려져서 스크롤까지 추가하게 됐습니다!
탑바는 스크롤에서 제외하면 아래 화면들이 올라가면서 탑바에 가려지는 문제가 생겨서 같이 올라가도록 해놨습니다..
어떻게 수정해야 할까요😢 다른 회원가입 화면들에서도 TextField가 많이 쓰이기 때문에 정해두고 가면 좋을 것 같습니다🙏
| onClick = { }, | ||
| modifier = Modifier | ||
| .align(Alignment.BottomCenter) | ||
| .padding(top = 20.dp), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 패딩은 왜 들어갔을까용?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
엇 빼겠습니다
| // TODO 상태 관리 따로 빼기 | ||
| val maxLength = 15 | ||
| var text by remember { mutableStateOf("") } | ||
| val warningState by remember { derivedStateOf { text.length > maxLength } } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isWarning 라는 변수명은 어떨까요?
| contentDescription = null, | ||
| modifier = Modifier.size(200.dp), | ||
| ) | ||
| Spacer(Modifier.weight(0.7f)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요 weight 는 어떤 의미인가용?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
두 Text와 Image영역 위 아래에 3:7로 여백을 주고 싶어서 저렇게 구현했습니다
정수로 쓰는게 좋을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
뭔가 디자인 의도는 이미지가 정중앙에 배치되어 있고, 그 위에 텍스트를 배치하는 느낌인데..
권장되진 않지만 지금 상황에서는 ConstraintLayout 를 사용하는 것도 방법일 수 있겠네요!
| val trainerText = stringResource(R.string.trainer) | ||
| val traineeText = stringResource(R.string.trainee) | ||
|
|
||
| var selectedRole by remember { mutableStateOf(trainerText) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
스트링 리소스를 기반으로 선택한 직군을 판단하고 있군요!
enum 으로 트레이너, 트레이니를 정의하면 좀 더 명확하게 와닿을 것 같아요!
도메인 모델에 들어갈 수 있겠네요!
| modifier = Modifier.align(Alignment.CenterHorizontally), | ||
| ) | ||
|
|
||
| Spacer(modifier = Modifier.weight(1.2f)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1.2..는 어떤 숫자일까요?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
얘도 위랑 동일한 이유로 설정해둔 값입니당..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제 생각에 이 부분은 SpaceBetween 만으로도 충분하지 않을까 싶은데 다른 이슈가 있나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
확인해보니 없습니다! SpaceBetween만 쓰도록 수정했습니다
| @@ -0,0 +1,6 @@ | |||
| package co.kr.tnt.domain.model | |||
|
|
|||
| enum class Role(val displayName: String) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'도메인' 모듈은 어떤 데이터를 담아야할까요?
displayName 은 어디서 보여주는 데이터인가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
비즈니스 로직들.. 프레젠테이션 계층은 모르는게 맞아요ㅠㅠ 수정하겠습니다..
|
|
||
| import co.kr.tnt.core.designsystem.R | ||
|
|
||
| sealed class RoleState( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Role 은 왜 삭제됐나요!? 우리 앱의 중요한 도메인 모델이지 않을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥹 다시 추가하겠습니다
hoyahozz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
도메인 모델인 Role 을 기반으로 RoleState 를 만들어주면 더 깔끔할 것 같아요 ㅎㅎ
우선 구현해주신 UI 모두 정상적으로 동작하는지 한 번 더 확인해보시고,
괜찮다면 머지하셔도 될 것 같습니다!
고생하셨습니다 ~ LGTM 👍👍👍
모두 다 잘 동작합니다!! 며칠간 꼼꼼한 리뷰 감사합니다😭 |

📝 작업 내용
구현한 화면
전달사항
📸 실행 화면
트레이너/트레이니 선택
default.mp4
트레이너 프로필 만들기
default.mp4
트레이너 프로필 생성 완료
default.mp4
🙆🏻 리뷰 요청 사항
트레이너 프로필 만들기 UI를 구현하면서 TextField에서 여러 줄 입력 시 키보드에 가려져 첫 번째 줄만 보이는 문제가 발생했습니다. 이를 해결하기 위해
BringIntoViewRequester를 사용하여 TextField가 항상 화면에 보이도록 처리했습니다.줄바꿈이 생길 때
BringIntoViewRequester를 활용해 해당 줄로 스크롤이 자동으로 내려가도록 구현했는데이 방식이 적절한지, 대체 방법이 있는지 확인 부탁드립니다!!
👀 레퍼런스