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

비밀번호 초기화 로직 변경 #375

Merged
merged 28 commits into from
Oct 30, 2024
Merged

Conversation

JuTaK97
Copy link
Collaborator

@JuTaK97 JuTaK97 commented Oct 26, 2024

플로우가 꽤 많이 바뀌기 때문에, 화면을 그냥 새로 만들었음

피그마 : https://www.figma.com/design/t9AoHBsLy0wjdIE8dcMtwp/SNUTT-Design-System?node-id=804-5177&node-type=section&t=WjpIw3JL9zvygVWl-0

뷰모델에서 UIState 제공하고 컴포저블은 이것만 바라보는 형태를 한번 제대로 갖춰보려고 해봤는데, 한번 읽어보고 의견 ㄱㄱ

- 사용성 개선 (자동 포커스, KeyboardActions)
- TextFieldValue 쓰는 EditText 로 교체해서, EditText에 초기값 있을 때 포커스 가면 커서가 맨 끝으로 갈 수 있도록 하기
- 비슷하게, 포커스 처리 + 커서 처리 + keyboardActions 처리 + imePadding
커서, 포커스 등 처리
code 도 함께 보내도록 변경한다.
AnimatedContent 안에 imePadding 을 주면 화면이 출렁인다
@JuTaK97 JuTaK97 requested a review from a team as a code owner October 26, 2024 13:21
Copy link
Collaborator

@plgafhd plgafhd left a comment

Choose a reason for hiding this comment

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

오 근데 진짜 괜찮아졌다
특히 ResetPasswordPage() 같은 곳 이렇게 안했을 때 어땠을지 생각해보면...

요거 잘 활용하면 ResetPasswordPage() Route-Page 두 개로 분리해서 페이지 전체를 Preview 볼수도 있겠다 실제로 NIA는 꽤 많은 컴포저블을 두 개로 분리해놨더라구 (대체로 ViewModel 필요하다 싶으면 그렇게 하는 듯)

underlineColor: Color = SNUTTColors.Gray200,
underlineColorFocused: Color = SNUTTColors.Black900,
underlineWidth: Dp = 1.dp,
clearFocusFlag: Boolean = false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

혹시 얘는 무슨 용도로 있는거야?
clearFocusFlag 값이 바뀌는 부분을 못찾았어...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

그거 EditText 에는 있을걸? 그 SearchPage 상단 EditText에서 x표시 누를때 포커스 해제 용도

Comment on lines +23 to +25
@Composable
fun IOSStyleTopBar(
modifier: Modifier = Modifier,
Copy link
Collaborator

Choose a reason for hiding this comment

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

ㅋㅋㅋㅋㅋ 이름도 이렇게 지은 김에
ios 화면 보니까 topbar 싹 가운데 정렬이던데
CenterAlignedTopAppBar 써보는거 어때?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

머터리얼 쓰기 싫어

Copy link
Collaborator

Choose a reason for hiding this comment

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

ㅋㅋㅋㅋㅋㅋ

Comment on lines +19 to +37

sealed class UIState {
data class CheckId(
val userId: String,
) : UIState()

data class EnterFullEmail(
val userId: String,
val maskedEmail: String,
val fullEmail: String,
) : UIState()

data class VerifyCode(
val fullEmail: String,
) : UIState()

data object EnterNewPassword : UIState()
}

Copy link
Collaborator

@plgafhd plgafhd Oct 28, 2024

Choose a reason for hiding this comment

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

오 이거 뭔가 신기하다
내가 봤던 UIState는, 주로 loading, error, success 이런거 구분해서
목록 같은거 가져올 때 쓰는것만 주로 보고 공부했었는데,
이렇게 쓰는것도 괜찮아보인다 계속 내려주면서 필요한 값 빼서 쓸수도 있고

사소한건데 혹시 이거 뷰모델 안에 있는 이유가 있어?
그냥 FindPasswordUiState 이런식으로 이름 지은 다음에 밖으로 빼는건 어떤가 싶어서

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FindPasswordPage의 상태의 stateHolder가 FindPasswordViewModel 인 거고, 이 상태를 생성하는것도, 제공하는것도, 변경하는것도 뷰모델이라서 상태 class가 뷰모델의 하위 클래스인게 맞지 않을까 생각했음

생긴게 다른이유

  1. 망할 apiOnError를 못 떼서, Error UIState를 만들고 싶어도 아직 못함
  2. 한 Page에서 4개의 퍼널이 있기 때문에 sealed class와 4개의 하위 클래스로 구성함

Copy link
Collaborator

@plgafhd plgafhd Oct 30, 2024

Choose a reason for hiding this comment

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

아하 사실 이건 구조적인? 문제보다는

@Composable
fun EnterFullEmailStep(
    uiState: FindPasswordViewModel.UIState.EnterFullEmail,
    notMyEmail: () -> Unit,
    onSubmitFullEmail: (String) -> Unit,
)

이런 곳에 FindPasswordViewModel.UIState 이렇게 되어 있는게 좀 거슬리지 않나 싶어서 물어본거였어
그렇다고 import 해버리면 그냥 UIState가 되니 이름만으로 구분이 안돼서
State 자체의 구조는 괜찮은것 같아

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아항 나는 일부러 이름 저렇게 보이면 좋아서 저렇게 의도한건데 ㅋㅋㅋ

Comment on lines +72 to +80
}
LaunchedEffect(timerState.currentValue) {
if (timerState.isEnded) {
errorDialogTitle = context.getString(R.string.find_password_enter_password_confirm_expired_alert)
showErrorDialog = true
}
}
LaunchedEffect(showCompleteDialog.value) {
if (showCompleteDialog.value) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

이거 원래 이런식으로 .value 이렇게 붙여야 됐던건가?
왜 낯설지...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

사실 별차이 없음 State 으로 넘겨서 그래

Comment on lines +110 to +116

EditText(
modifier = Modifier
.fillMaxWidth()
.focusRequester(focusRequester),
value = newPasswordField,
onValueChange = { newPasswordField = it },
Copy link
Collaborator

Choose a reason for hiding this comment

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

혹시 다른 곳은 EditTextFieldValue (요거 이름도 살짝 헷갈리는데 대체할만할지도?) 쓰는데
여기는 EditText로 쓴 이유가 있나용
두 개 차이가 value랑 onValueChanged 밖에 없는것 같아서, 다른 곳에서 그랬던 것처럼 newPasswordField를 TextFieldValue로 감쌀 수 있지 않나 싶어서

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

커서 맨끝으로 오게하는 기능이 필요 없어서ㅋㅋ
TextfieldValue보다 string이 코드 짧으니까..

Comment on lines +56 to +67
backButtonText = when (uiState) {
is CheckId -> stringResource(R.string.find_password_back_login)
is EnterFullEmail -> stringResource(R.string.find_password_back_check_id)
is VerifyCode -> stringResource(R.string.find_password_back_check_email)
is EnterNewPassword -> stringResource(R.string.find_password_back_initial)
},
) {
if (uiState is CheckId) {
navController.popBackStack()
} else {
viewModel.goToPreviousStep()
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

혹시 이거 uiState랑 popBackStack, goToPreviousStep 이런식으로 넘기고 어떤 동작/어떤 ui를 띄울지를 IOSStyleTopBar 내부에서 판단하게 할 수도 있으려나...?
정확히는 가능하긴 할텐데 그게 괜찮은 방법인지 궁금해 TopBar가 하는 동작을 Page가 모르게 한다는 정도의 의미가 있는데, 단점도 분명하긴 한것 같아서

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

페이지의 UI 로직인데 탑바가 처리할 필요는 없지 않을까?
탑바는 어디까지나 공용 컴포넌트니까

Copy link
Collaborator

Choose a reason for hiding this comment

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

그렇긴 하네 탑바는 공용이니까

Comment on lines +91 to +93

EditText(
modifier = Modifier
Copy link
Collaborator

Choose a reason for hiding this comment

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

오 근데 좀 더 보니까 여기도 EditText네
급하게 보느라 어떤 차이인지 자세히는 못보는 중...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

커밋메시지에 써있음

@JuTaK97 JuTaK97 enabled auto-merge October 30, 2024 14:45
@JuTaK97 JuTaK97 disabled auto-merge October 30, 2024 14:45
@JuTaK97 JuTaK97 enabled auto-merge (squash) October 30, 2024 14:45
@JuTaK97 JuTaK97 merged commit daa3c16 into develop Oct 30, 2024
3 checks passed
@JuTaK97 JuTaK97 deleted the jutak/password-reset-refactor branch October 30, 2024 14:54
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.

2 participants