-
Notifications
You must be signed in to change notification settings - Fork 1
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
[UI/#159][FEAT/#160] mypage / 전체 로직 구현 #188
Conversation
# Conflicts: # feature/main/src/main/java/com/boostcamp/mapisode/main/MainNavigator.kt
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.
나중에 다시 봐볼게요 리뷰 반영해서 재요청해주세요
sealed interface MypageRoute : Route { | ||
@Serializable | ||
data object ProfileEdit : MypageRoute | ||
} |
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.
별도로 Route 정의한게 이미 있습니다. 왜 만드신거죠?
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.
프로필을 변경하는 화면으로 가는 route가 필요해서 뚫었습니다. 다른 좋은 방법이 있나요???
var endSplash by mutableStateOf(false) | ||
|
||
fun navigateToLogin() { | ||
navController.navigate(Route.Auth) | ||
} | ||
|
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.
endSplash는 제거했습니다.
navigateToLogin은 로그아웃시 필요해서 popbackstack하는 코드로 바꾸었습니다.
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.
popupto 사용했습니다.
<?xml version="1.0" encoding="utf-8"?> | ||
<manifest xmlns:android="http://schemas.android.com/apk/res/android"> | ||
|
||
<uses-permission android:name="android.permission.INTERNET" /> | ||
</manifest> |
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.
app 모듈에 있는데 왜 추가하셨나요?
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 fun initState() { | ||
viewModelScope.launch { | ||
userPreferenceDataStore.getUserPreferencesFlow().first().let { userPreferences -> |
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.
깜빡했네여 ㅜㅜ 말씀해주셔서 감사합니다! 수정했습니다!
} | ||
|
||
private fun openPrivacyPolicy(context: Context, useCustomTab: Boolean) { | ||
val url = "https://pricey-visitor-e41.notion.site/14e94239a962805b9dadf87c68353909?pvs=4" |
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.
url 같은 경우도 로컬 프로퍼티에 저장하는게 좋습니다.
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 fun initState() { | ||
viewModelScope.launch { | ||
userPreferenceDataStore.getUserPreferencesFlow().first().let { userPreferences -> | ||
intent { | ||
copy( | ||
isLoading = false, | ||
uid = userPreferences.userId ?: throw Exception("UserId is null"), | ||
name = userPreferences.username ?: throw Exception("Username is null"), | ||
profileUrl = userPreferences.profileUrl | ||
?: throw Exception("ProfileUrl is null"), | ||
) | ||
} | ||
} | ||
} | ||
} |
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.
여기도 빠르렸네요 ㅠ 귀찮게 해드려 죄송합니다 ㅠ
viewModel: MypageViewModel = hiltViewModel(), | ||
) { | ||
val context = LocalContext.current | ||
val googleOauth = GoogleOauth(context) |
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.
조언 감사합니다!
이번주 내로 반영해보겠습니다!
titleText = context.getString(R.string.mypage_withdrawal_title), | ||
contentText = context.getString(R.string.mypage_withdrawal_message), | ||
confirmText = context.getString(R.string.mypage_withdrawal_confirm), | ||
cancelText = "취소", |
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.
네넵!!
수정했습니다
viewModel.onIntent(MypageIntent.ConfirmClick(googleOauth)) | ||
} | ||
|
||
false -> {} |
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.
false일 때 처리 해주세요
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 문으로 변경했습니다. 계정 삭제에서 취소했을 때, 보통 아무것도 안 뜨는 것 같다고 생각했기 때문입니다.
감사합니다~
try { | ||
if (useCustomTab) { | ||
val customTabsIntent = CustomTabsIntent.Builder() | ||
.setShowTitle(true) | ||
.build() | ||
customTabsIntent.launchUrl(context, Uri.parse(url)) | ||
} else { | ||
val intent = Intent(Intent.ACTION_VIEW, Uri.parse(url)) | ||
context.startActivity(intent) |
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.
이 작업이 뷰모델에서 일어나면 안되죠. context도 뷰모델에서 사용하는 것을 지양해주세요
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.
말씀해주셔서 감사합니다!
성공해서 그냥 넘어갔었네요ㅠ
감사합니다!! sideeffect에서 처리하도록 했습니다.
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 fun onInit() { | ||
handleAutoLogin() | ||
} |
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.
그러게요 ㅋㅋㅋㅋㅋ
원래 저기서 intent를 호출하는 로직이 있었는데 불필요해서 삭제했습니다.
onInit은 아무리 봐도 쓸모가 없네요 ㅋㅋㅋ
감사합니다!!!!!!!!!!!!!!!!!!!🤩
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.
고생많으셨어요~ repoImpl create, update user 함수 손봐주시면 되겠습니다
PRIVACY_POLICY: ${{ secrets.PRIVACY_POLICY }} | ||
run: | | ||
echo NAVER_MAP_CLIENT_ID=$NAVER_MAP_CLIENT_ID > ./local.properties | ||
echo NAVER_MAP_CLIENT_SECRET=$NAVER_MAP_CLIENT_SECRET >> ./local.properties | ||
echo GOOGLE_WEB_CLIENT_ID=$GOOGLE_WEB_CLIENT_ID >> ./local.properties | ||
echo PRIVACY_POLICY=$PRIVACY_POLICY >> ./local.properties |
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.
local properties 추가 하셨나보네요!!
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.
넵 ㅎㅎ
태희님의 추천으로 local properties에 추가했습니다.
override suspend fun updateUserNameAndProfileUrl( | ||
uid: String, | ||
userName: String, | ||
profileUrl: String, | ||
) { | ||
val userDocument = userCollection.document(uid) | ||
|
||
try { | ||
userDocument.update( | ||
mapOf( | ||
"name" to userName, | ||
"profileUrl" to profileUrl, | ||
), | ||
).await() | ||
} catch (e: Exception) { | ||
throw Exception("Failed to update user", e) | ||
} | ||
} |
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.
여기랑 createUser 함수 다시보니까 firebase storage에 저장/덮어씌우기 하는 코드가 안보이네요.
로컬에서의 url은 디바이스에 저장된 경로고, 서버에 올라가야할 url은 firebase storage이에요.
coil에서는 두가지 전부 지원을 해서 get/create 에서 하나의 url 프로퍼티를 공용하고 있는데 update같은 경우에는 프로퍼티가 두가지여야 해요.
업데이트에 한해서는 서버 url, 로컬url 두가지 프로퍼티를 갖고 대조해서 바뀐 이미지인지 아닌지 구분하고 바뀌었다면 storage에 업로드 후 user collection에 저장해야 해요.
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로 올리겠습니다!
LaunchedEffect(Unit) { | ||
viewModel.sideEffect.collect { sideEffect -> | ||
when (sideEffect) { | ||
is ProfileEditSideEffect.Idle -> {} |
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.
여기서 Idle이 필요할까요?
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.
필요 없는 것 같아서 삭제를 고민하고 있었습니다!
삭제하겠습니다!
📍 Work Description
📸 Screenshot
6.mp4
📢 To Reviewers
⏲️Time