-
Notifications
You must be signed in to change notification settings - Fork 9
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/#928] 로그인 중앙화 UI 구현 #1008
Conversation
modifier = Modifier.padding(horizontal = 8.dp) | ||
) | ||
HorizontalDivider( | ||
thickness = 0.6.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.
여기도 마찬가지
private val _action = MutableStateFlow<Boolean?>(null) | ||
val action: StateFlow<Boolean?> = _action.asStateFlow() |
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.
혹시 이건 StateFlow로 구현한 이유가 있을까요? 이건 이벤트쪽인 것 같아서 State보단 Shared로 구현하는게 더 자연스러울 것 같아서요...!!
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.
SharedFlow로 구현하면 이를 LaunchedEffectf로 수집해야하는데 LaunchedEffect 안에서는 컴포저블 함수를 사용할 수가 없더라구요..!
다이얼로그는 컴포저블 함수이기 때문에 일단 StateFlow를 사용했습니다ㅜㅜ 다른 방법이 있을까요..?
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.
아 그런 요구사항이 있었구만유, 그런 경우에는 action이란 표현 대신에 isLoginErrorPopupVisible 정도로 축약해도 좋을 것 같아요. 아니면, 이건 UI단에서만 알고 있어도 되는 정보이니, setContent안에
var isErrorDialogVisible by remember { mutableStateOf(false) }
로 사용해도 괜찮을듯합니다! 상태값 업데이트 코드는 AuthUiEvent가 Failure일 때 LaunchedEffect에서 작성하면 될 것 같구요
app/src/main/java/org/sopt/official/feature/auth/component/AuthButton.kt
Outdated
Show resolved
Hide resolved
if (text.isEmpty()) | ||
Text( | ||
text = hintText, | ||
color = Gray100, | ||
style = SoptTheme.typography.body14M | ||
) |
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.
if (text.isEmpty()) | |
Text( | |
text = hintText, | |
color = Gray100, | |
style = SoptTheme.typography.body14M | |
) | |
if (text.isEmpty()) { | |
Text( | |
text = hintText, | |
color = Gray100, | |
style = SoptTheme.typography.body14M | |
) | |
} |
@Composable | ||
internal fun AuthTextField( | ||
text: String, | ||
hintText: 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.
https://m3.material.io/components/text-fields/guidelines
labelText(or placeHolder)라는 네이밍은 어떨까요? Material3 보니까 기본 placeHolder가 labelText로 되어있네요
app/src/main/java/org/sopt/official/feature/auth/component/LoginErrorDialog.kt
Show resolved
Hide resolved
app/src/main/java/org/sopt/official/feature/auth/component/PhoneCertification.kt
Show resolved
Hide resolved
...ain/java/org/sopt/official/feature/auth/feature/certificatemember/CertificateMemberScreen.kt
Show resolved
Hide resolved
val onShowSnackBar: () -> Unit = { | ||
coroutineScope.launch { | ||
snackBarHostState.showSnackbar("인증번호가 전송되었어요.") | ||
} | ||
} |
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.
val onShowSnackBar: () -> Unit = { | |
coroutineScope.launch { | |
snackBarHostState.showSnackbar("인증번호가 전송되었어요.") | |
} | |
} | |
val onShowSnackBar: () -> Unit = remember { { | |
coroutineScope.launch { | |
snackBarHostState.showSnackbar("인증번호가 전송되었어요.") | |
} | |
} } |
(혹시 모르는데) 이 함수가 리컴포지션될때마다 매번 리컴포지션 범위에 포함되어서 다시 함수객체가 만들어질 수 있으니 remember로 감싸주면 좋을 것 같아요
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.
고생하셨어요 🙇🏻 리뷰 남겼습니다
코드리뷰 반영했습니다! 확인부탁드려요! @l2hyunwoo |
LaunchedEffect(true) { | ||
if (dataStore.accessToken.isNotEmpty()) { | ||
startActivity( | ||
HomeActivity.getIntent(context, HomeActivity.StartArgs(UserStatus.of(dataStore.userStatus))) | ||
) | ||
} | ||
} |
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.
토큰 있는 경우에는 어떻게 동작하나요...?
initAnimation() | ||
collectUiEvent() | ||
} | ||
val action by viewModel.loginDialogAction.collectAsStateWithLifecycle() |
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.
val action by viewModel.loginDialogAction.collectAsStateWithLifecycle() | |
val loginDialogAction by viewModel.loginDialogAction.collectAsStateWithLifecycle() |
- 저게 액션일까요?
- collectAsStateWithLifecycle이 필요할까?
- ViewModel에 변수로 선언해야하는가?
enableLights(false) | ||
enableVibration(false) | ||
lockscreenVisibility = NotificationCompat.VISIBILITY_PUBLIC | ||
(getSystemService(NOTIFICATION_SERVICE) as NotificationManager).createNotificationChannel(this) |
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.
(getSystemService(NOTIFICATION_SERVICE) as NotificationManager).createNotificationChannel(this) | |
getSystemService<NotificationManager>?.createNotificationChannel(this) |
authDataSource = object : PlaygroundAuthDatasource { | ||
override suspend fun oauth(code: String): Result<OAuthToken> { | ||
return kotlin.runCatching { | ||
authService | ||
.authenticate(AuthRequest(code, dataStore.pushToken)) | ||
.toOAuthToken() | ||
} | ||
} | ||
} |
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.
오래된 잔재....이건 제가 건드려야될 것 같군요 ㅠ
containerColor: Color = White, | ||
contentColor: Color = Black, | ||
isEnabled: Boolean = true, | ||
disabledContainerColor: Color = Black80, | ||
disabledContentColor: Color = Gray60, |
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.
containerColor: Color = White, | |
contentColor: Color = Black, | |
isEnabled: Boolean = true, | |
disabledContainerColor: Color = Black80, | |
disabledContentColor: Color = Gray60, | |
isEnabled: Boolean = true, | |
containerColor: Color = White, | |
contentColor: Color = Black, | |
disabledContainerColor: Color = Black80, | |
disabledContentColor: Color = Gray60, |
~Defaults object를 활용해서 컬러 시스템을 하나의 객체 안에서 관리할 수 있게하는 패턴이 있습니당!
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.
This reverts commit d719334.
What is this issue?
Screenshot
AuthActivity
CertificateMemberScreen
Screen_Recording_20241225_070137_SOPT.DEBUG.mp4
Screen_Recording_20241225_073953_SOPT.DEBUG.mp4
AuthErrorScreen
CertificateAccountScreen
ChangeAccountScreen
To Reviewers